Canvas foundation: SVG viewport, pan/zoom/pinch, geometry lib (#9) #29

Merged
steve merged 2 commits from phase-3-canvas into main 2026-07-19 00:18:28 +00:00
Owner

Closes #9. Phase 3, the technically-riskiest slice — built against mock data so #11 (field editor) and #15 (plops) only add interactions on top.

What's here

  • lib/geometry.ts (+ vitest) — world↔screen and object-local↔world transforms (center + rotation; plops live in the local frame), clampScale, zoomViewportAt (the "keep the point under the cursor stationary" math for wheel/pinch), zoomToFitRect.
  • editor/store.ts — a Zustand store for ephemeral editor state (viewport, selection, focusedObjectId); selection interactions grow in #11.
  • editor/useViewport.ts@use-gesture/react: wheel + pinch zoom toward the pointer, drag-pan on empty space, scale clamped to [0.05, 20] px/cm, and an animated fitToRect (eased rAF tween). The svg sets touch-action: none so the browser doesn't fight gestures.
  • editor/ObjectShape.tsx — rect/circle centered and rotated about center, kind-default colors (overridable), name label; vector-effect=non-scaling-stroke so outlines stay 1px at any zoom.
  • editor/GardenCanvas.tsx — full-size <svg> with one world <g transform="translate(tx,ty) scale(s)">, a fading 1 m grid, the garden boundary, z-ordered objects; ResizeObserver auto-fit, a px/cm readout, and a Fit control.
  • GardenEditorPage renders the canvas with a mock scene (#11 swaps in the garden's /full payload). New deps: zustand, @use-gesture/react, vitest (+ test scripts).

Verification

  • npx vitest run17 green: geometry round-trips, 90° rotation (clockwise, y-down), cursor-anchored zoom stays put, zoomToFitRect centers + clamps; units conversions/formatting.
  • tsc --noEmit clean; vite build succeeds.
  • Real browser (embedded binary): auto-fit frames the 12 m × 8 m mock garden; a dispatched wheel keeps the world point under the cursor stationary (sub-cm drift); a real drag pans (translate moved, scale unchanged); Fit animates back to frame. Screenshot shared in-session.
  • Phone/precise-touch pinch is best confirmed on a device — the pinch handler is standard use-gesture with a scale-seeded from and origin-anchored zoom.

Out of scope

  • Server data, palette, move/resize/rotate (#11). Plops + semantic-zoom bands (#15). Polygon shapes (post-v1).

🤖 Generated with Claude Code

Closes #9. Phase 3, the technically-riskiest slice — built against **mock data** so #11 (field editor) and #15 (plops) only add interactions on top. ## What's here - **`lib/geometry.ts`** (+ vitest) — world↔screen and object-local↔world transforms (center + rotation; plops live in the local frame), `clampScale`, `zoomViewportAt` (the "keep the point under the cursor stationary" math for wheel/pinch), `zoomToFitRect`. - **`editor/store.ts`** — a Zustand store for ephemeral editor state (viewport, selection, `focusedObjectId`); selection interactions grow in #11. - **`editor/useViewport.ts`** — `@use-gesture/react`: wheel + pinch zoom **toward the pointer**, drag-pan on empty space, scale clamped to `[0.05, 20]` px/cm, and an **animated `fitToRect`** (eased rAF tween). The svg sets `touch-action: none` so the browser doesn't fight gestures. - **`editor/ObjectShape.tsx`** — rect/circle centered and rotated about center, kind-default colors (overridable), name label; `vector-effect=non-scaling-stroke` so outlines stay 1px at any zoom. - **`editor/GardenCanvas.tsx`** — full-size `<svg>` with one world `<g transform="translate(tx,ty) scale(s)">`, a fading 1 m grid, the garden boundary, z-ordered objects; ResizeObserver auto-fit, a px/cm readout, and a **Fit** control. - `GardenEditorPage` renders the canvas with a mock scene (#11 swaps in the garden's `/full` payload). New deps: `zustand`, `@use-gesture/react`, `vitest` (+ `test` scripts). ## Verification - `npx vitest run` — **17 green**: geometry round-trips, 90° rotation (clockwise, y-down), cursor-anchored zoom stays put, `zoomToFitRect` centers + clamps; units conversions/formatting. - `tsc --noEmit` clean; `vite build` succeeds. - **Real browser** (embedded binary): auto-fit frames the 12 m × 8 m mock garden; a dispatched wheel keeps the world point under the cursor stationary (sub-cm drift); a **real drag pans** (translate moved, scale unchanged); **Fit** animates back to frame. Screenshot shared in-session. - Phone/precise-touch pinch is best confirmed on a device — the pinch handler is standard use-gesture with a scale-seeded `from` and origin-anchored zoom. ## Out of scope - Server data, palette, move/resize/rotate (#11). Plops + semantic-zoom bands (#15). Polygon shapes (post-v1). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-19 00:00:12 +00:00
Add canvas foundation: SVG viewport, pan/zoom/pinch, geometry lib (#9)
Build image / build-and-push (push) Successful in 16s
Gadfly review (reusable) / review (pull_request) Successful in 9m31s
Adversarial Review (Gadfly) / review (pull_request) Successful in 9m31s
30b36b7033
The editor's technically-riskiest slice, built against mock data so #11
only adds interactions on top.

- lib/geometry.ts (+ vitest): world<->screen and object-local<->world
  transforms, clampScale, zoomViewportAt (the wheel/pinch "keep the point
  under the cursor stationary" math), zoomToFitRect. 11 geometry tests
  (round-trips, rotation, cursor-anchored zoom, fit) + 6 units tests.
- editor/store.ts: Zustand store for ephemeral editor state — viewport,
  selection, focusedObjectId (selection interactions grow in #11).
- editor/useViewport.ts: @use-gesture wiring — wheel + pinch zoom toward
  the pointer, drag-pan, scale clamped to [0.05, 20] px/cm, and an animated
  fitToRect (eased rAF tween). touch-action:none so the browser doesn't
  fight gestures.
- editor/ObjectShape.tsx: rect/circle centered + rotated, kind default
  colors, name label; non-scaling strokes so outlines stay 1px at any zoom.
- editor/GardenCanvas.tsx: full-size svg, single world <g>, fading 1m grid,
  garden boundary, z-ordered objects; ResizeObserver-driven auto-fit, a
  zoom readout, and a Fit control. Deps: zustand, @use-gesture/react,
  vitest (+ test scripts).
- GardenEditorPage renders the canvas with a mock scene (#11 swaps in
  /full).

Verified in a real browser: wheel zoom keeps the world point under the
cursor stationary (sub-cm drift on a 12m garden), a real drag pans, and
Fit animates to frame the garden. tsc + vitest green.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-19 00:09:44Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

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

opencode/glm-5.2:cloud · opencode — done

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

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

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

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:09:44Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — No material issues found #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-19 00:09:44 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 22 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** — 22 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +10,4 @@
/**
* The editor canvas: a full-size SVG with a single world <g> that pan/zoom/pinch
* transforms. Renders a fading 1 m grid, the garden boundary, and its objects.

Doc says "fading 1 m grid" but grid only toggles with constant opacity (no fade)

maintainability · flagged by 1 model

  • web/src/editor/GardenCanvas.tsx:13 — doc comment says "a fading 1 m grid", but the grid just toggles on/off via showGrid (GRID_CM * viewport.scale >= GRID_MIN_CELL_PX, line 52) with constant strokeOpacity={0.18} (line 74). There is no fade. The comment will mislead maintainers. Either implement a fade (opacity as a function of cell px) or drop "fading". Verified.

🪰 Gadfly · advisory

⚪ **Doc says "fading 1 m grid" but grid only toggles with constant opacity (no fade)** _maintainability · flagged by 1 model_ - **`web/src/editor/GardenCanvas.tsx:13`** — doc comment says "a fading 1 m grid", but the grid just toggles on/off via `showGrid` (`GRID_CM * viewport.scale >= GRID_MIN_CELL_PX`, line 52) with constant `strokeOpacity={0.18}` (line 74). There is no fade. The comment will mislead maintainers. Either implement a fade (opacity as a function of cell px) or drop "fading". Verified. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +18,4 @@
const svgRef = useRef<SVGSVGElement>(null)
const containerRef = useRef<HTMLDivElement>(null)
const [size, setSize] = useState<Size>({ w: 0, h: 0 })
const fittedRef = useRef(false)

🟠 fittedRef prevents re-fit when garden prop changes

maintainability · flagged by 1 model

  • web/src/editor/GardenCanvas.tsx:21,45-50fittedRef gates the auto-fit effect so it only fires once per component lifetime. If the garden prop changes (which #11 will support), the canvas silently stays framed for the old garden. Track the last fitted garden ID and reset the flag when garden.id changes, or drive fit state from a stable key.

🪰 Gadfly · advisory

🟠 **fittedRef prevents re-fit when garden prop changes** _maintainability · flagged by 1 model_ - `web/src/editor/GardenCanvas.tsx:21,45-50` — `fittedRef` gates the auto-fit effect so it only fires once per component lifetime. If the `garden` prop changes (which #11 will support), the canvas silently stays framed for the old garden. Track the last fitted garden ID and reset the flag when `garden.id` changes, or drive fit state from a stable key. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +43,4 @@
// Frame the garden the first time we know the canvas size.
useEffect(() => {
if (!fittedRef.current && size.w > 0 && size.h > 0) {

🟠 Auto-fit effect lacks garden dimension validation, allowing NaN viewport

error-handling · flagged by 1 model

  • web/src/editor/store.ts:24setViewport accepts any Viewport without clamping scale to [MIN_SCALE, MAX_SCALE]. A direct call with scale: 0 (or NaN/negative) corrupts the canvas permanently because downstream screenToWorld divides by vp.scale, yielding Infinity/NaN. The store should enforce the invariant it exports, either by clamping inside setViewport or by rejecting non-finite values. - web/src/editor/useViewport.ts:33-34toLocal silently falls back to…

🪰 Gadfly · advisory

🟠 **Auto-fit effect lacks garden dimension validation, allowing NaN viewport** _error-handling · flagged by 1 model_ - **`web/src/editor/store.ts:24`** — `setViewport` accepts any `Viewport` without clamping `scale` to `[MIN_SCALE, MAX_SCALE]`. A direct call with `scale: 0` (or `NaN`/negative) corrupts the canvas permanently because downstream `screenToWorld` divides by `vp.scale`, yielding `Infinity`/`NaN`. The store should enforce the invariant it exports, either by clamping inside `setViewport` or by rejecting non-finite values. - **`web/src/editor/useViewport.ts:33-34`** — `toLocal` silently falls back to… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +66,4 @@
{showGrid && (
<>
<defs>
<pattern id="garden-grid" width={GRID_CM} height={GRID_CM} patternUnits="userSpaceOnUse">

🟠 Hardcoded SVG pattern ID risks collision across canvas instances

error-handling, maintainability · flagged by 2 models

  • web/src/editor/GardenCanvas.tsx:69 — Hardcoded SVG <pattern id="garden-grid"> will collide if multiple GardenCanvas instances ever render on the same page (e.g. side-by-side comparisons, future features, tests). Use React's useId() to generate a unique pattern ID per instance and reference it with fill={url(#${gridId})}.

🪰 Gadfly · advisory

🟠 **Hardcoded SVG pattern ID risks collision across canvas instances** _error-handling, maintainability · flagged by 2 models_ - **`web/src/editor/GardenCanvas.tsx:69`** — Hardcoded SVG `<pattern id="garden-grid">` will collide if multiple `GardenCanvas` instances ever render on the same page (e.g. side-by-side comparisons, future features, tests). Use React's `useId()` to generate a unique pattern ID per instance and reference it with `fill={`url(#${gridId})`}`. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +108,4 @@
<button
type="button"
onClick={() => size.w > 0 && fitToRect(gardenRect, size)}
className="rounded-md border border-border bg-surface/90 px-3 py-1.5 text-sm font-medium text-fg shadow-sm outline-none backdrop-blur transition-colors hover:bg-border/50 focus-visible:ring-2 focus-visible:ring-accent/40"

🟡 Fit button duplicates Button/buttonClasses styling instead of reusing the shared component

maintainability · flagged by 1 model

  • web/src/editor/GardenCanvas.tsx:111 — The "Fit" button hand-rolls its own Tailwind class string instead of reusing the shared Button/buttonClasses helper (web/src/components/ui/Button.tsx:7-17), which already defines this exact "ghost" styling. The rest of the app follows the shared-button convention (GardensPage.tsx uses <Button>). This new component is the one place that breaks that pattern, so a future button-style tweak would need updating in two places. Fix: `<button className…

🪰 Gadfly · advisory

🟡 **Fit button duplicates Button/buttonClasses styling instead of reusing the shared component** _maintainability · flagged by 1 model_ - `web/src/editor/GardenCanvas.tsx:111` — The "Fit" button hand-rolls its own Tailwind class string instead of reusing the shared `Button`/`buttonClasses` helper (`web/src/components/ui/Button.tsx:7-17`), which already defines this exact "ghost" styling. The rest of the app follows the shared-button convention (`GardensPage.tsx` uses `<Button>`). This new component is the one place that breaks that pattern, so a future button-style tweak would need updating in two places. Fix: `<button className… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +3,4 @@
// Default fills by kind (overridable per object via object.color). Muted, earthy
// tones so plops (added in #15) read clearly on top.
const kindColors: Record<string, string> = {

Duplicated #8a8a8a default fill (kindColors entry + fillFor fallback)

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **Duplicated #8a8a8a default fill (kindColors entry + fillFor fallback)** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +24,4 @@
* vector-effect=non-scaling-stroke to stay a constant pixel width at any zoom.
* Full move/resize/rotate interactions come in #11.
*/
export function ObjectShape({

🟡 ObjectShape lacks memoization, causing unnecessary re-render of every object on each pan/zoom/pinch/fit-animation frame

performance · flagged by 2 models

  • web/src/editor/GardenCanvas.tsx:97-99 / web/src/editor/ObjectShape.tsx:27ObjectShape isn't memoized, so every object in the scene re-renders on every pan/wheel/pinch tick and on every frame of the fitToRect animation tween. GardenCanvas subscribes to viewport from the zustand store (GardenCanvas.tsx:23), and useViewport's onDrag/onWheel/onPinch handlers call setViewport on essentially every gesture event (useViewport.ts:42-58), while fitToRect's rAF loop calls it…

🪰 Gadfly · advisory

🟡 **ObjectShape lacks memoization, causing unnecessary re-render of every object on each pan/zoom/pinch/fit-animation frame** _performance · flagged by 2 models_ - `web/src/editor/GardenCanvas.tsx:97-99` / `web/src/editor/ObjectShape.tsx:27` — `ObjectShape` isn't memoized, so every object in the scene re-renders on every pan/wheel/pinch tick and on every frame of the `fitToRect` animation tween. `GardenCanvas` subscribes to `viewport` from the zustand store (`GardenCanvas.tsx:23`), and `useViewport`'s `onDrag`/`onWheel`/`onPinch` handlers call `setViewport` on essentially every gesture event (`useViewport.ts:42-58`), while `fitToRect`'s rAF loop calls it… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +34,4 @@
onSelect: (id: number) => void
}) {
const fill = fillFor(object)
const halfW = object.widthCm / 2

🟠 ObjectShape renders invalid SVG for zero or negative dimensions

error-handling, maintainability · flagged by 3 models

  • web/src/editor/store.ts:24setViewport accepts any Viewport without clamping scale to [MIN_SCALE, MAX_SCALE]. A direct call with scale: 0 (or NaN/negative) corrupts the canvas permanently because downstream screenToWorld divides by vp.scale, yielding Infinity/NaN. The store should enforce the invariant it exports, either by clamping inside setViewport or by rejecting non-finite values. - web/src/editor/useViewport.ts:33-34toLocal silently falls back to…

🪰 Gadfly · advisory

🟠 **ObjectShape renders invalid SVG for zero or negative dimensions** _error-handling, maintainability · flagged by 3 models_ - **`web/src/editor/store.ts:24`** — `setViewport` accepts any `Viewport` without clamping `scale` to `[MIN_SCALE, MAX_SCALE]`. A direct call with `scale: 0` (or `NaN`/negative) corrupts the canvas permanently because downstream `screenToWorld` divides by `vp.scale`, yielding `Infinity`/`NaN`. The store should enforce the invariant it exports, either by clamping inside `setViewport` or by rejecting non-finite values. - **`web/src/editor/useViewport.ts:33-34`** — `toLocal` silently falls back to… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +57,4 @@
r={halfW}
fill={fill}
fillOpacity={0.85}
stroke={selected ? '#2f7a3e' : '#00000033'}

🟠 Circle shapes render radius from widthCm only, silently ignoring heightCm even though the backend allows them to differ

correctness · flagged by 1 model

  • web/src/editor/ObjectShape.tsx:60 — circle rendering uses only halfW (r={halfW}) for the radius and silently ignores heightCm. Cross-checked internal/service/objects.go:223-256 (finalizeObject): it validates WidthCM and HeightCM independently via validDimensionCM with no constraint that they be equal for ShapeCircle objects. So a backend-valid object with shape=circle, widthCm=100, heightCm=140 would render as a perfect circle of radius 50, silently discarding the height.…

🪰 Gadfly · advisory

🟠 **Circle shapes render radius from widthCm only, silently ignoring heightCm even though the backend allows them to differ** _correctness · flagged by 1 model_ - `web/src/editor/ObjectShape.tsx:60` — circle rendering uses only `halfW` (`r={halfW}`) for the radius and silently ignores `heightCm`. Cross-checked `internal/service/objects.go:223-256` (`finalizeObject`): it validates `WidthCM` and `HeightCM` independently via `validDimensionCM` with no constraint that they be equal for `ShapeCircle` objects. So a backend-valid object with `shape=circle, widthCm=100, heightCm=140` would render as a perfect circle of radius 50, silently discarding the height.… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +67,4 @@
y={-halfH}
width={object.widthCm}
height={object.heightCm}
rx={Math.min(halfW, halfH) * 0.06}

Uncommented magic factors 0.06 (rx) and 0.28 (font) lack rationale

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **Uncommented magic factors 0.06 (rx) and 0.28 (font) lack rationale** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +15,4 @@
selectedId: number | null
select: (id: number | null) => void
focusedObjectId: number | null

🟡 focusedObjectId/setFocusedObject declared but unused (dead state until #15)

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **focusedObjectId/setFocusedObject declared but unused (dead state until #15)** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +21,4 @@
export const useEditorStore = create<EditorState>((set) => ({
viewport: { tx: 0, ty: 0, scale: 1 },
setViewport: (next) =>

🟠 setViewport accepts unclamped scale, allowing division-by-zero corruption

error-handling · flagged by 1 model

  • web/src/editor/store.ts:24setViewport accepts any Viewport without clamping scale to [MIN_SCALE, MAX_SCALE]. A direct call with scale: 0 (or NaN/negative) corrupts the canvas permanently because downstream screenToWorld divides by vp.scale, yielding Infinity/NaN. The store should enforce the invariant it exports, either by clamping inside setViewport or by rejecting non-finite values. - web/src/editor/useViewport.ts:33-34toLocal silently falls back to…

🪰 Gadfly · advisory

🟠 **setViewport accepts unclamped scale, allowing division-by-zero corruption** _error-handling · flagged by 1 model_ - **`web/src/editor/store.ts:24`** — `setViewport` accepts any `Viewport` without clamping `scale` to `[MIN_SCALE, MAX_SCALE]`. A direct call with `scale: 0` (or `NaN`/negative) corrupts the canvas permanently because downstream `screenToWorld` divides by `vp.scale`, yielding `Infinity`/`NaN`. The store should enforce the invariant it exports, either by clamping inside `setViewport` or by rejecting non-finite values. - **`web/src/editor/useViewport.ts:33-34`** — `toLocal` silently falls back to… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +15,4 @@
rotationDeg: number
zIndex: number
color?: string | null
plantable: boolean

🟡 EditorObject.plantable is carried in mock data but never read by the canvas

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **EditorObject.plantable is carried in mock data but never read by the canvas** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +24,4 @@
name: string
widthCm: number
heightCm: number
unitPref: 'metric' | 'imperial'

EditorGarden.unitPref re-declares the UnitPref union instead of importing it from lib/units.ts

maintainability · flagged by 1 model

  • web/src/editor/types.ts:27EditorGarden.unitPref is typed as the inline literal 'metric' | 'imperial' instead of reusing the existing UnitPref type exported from web/src/lib/units.ts:5, which web/src/lib/gardens.ts:7,40 already imports for the same purpose. Duplicated source of truth for a domain-relevant union. Fix: import type { UnitPref } from '@/lib/units' and use it in place of the inline literal.

🪰 Gadfly · advisory

⚪ **EditorGarden.unitPref re-declares the UnitPref union instead of importing it from lib/units.ts** _maintainability · flagged by 1 model_ - `web/src/editor/types.ts:27` — `EditorGarden.unitPref` is typed as the inline literal `'metric' | 'imperial'` instead of reusing the existing `UnitPref` type exported from `web/src/lib/units.ts:5`, which `web/src/lib/gardens.ts:7,40` already imports for the same purpose. Duplicated source of truth for a domain-relevant union. Fix: `import type { UnitPref } from '@/lib/units'` and use it in place of the inline literal. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +7,4 @@
const FIT_PADDING = 32 // px margin around a fitted rect
const FIT_DURATION_MS = 350
const easeInOutCubic = (t: number) => (t < 0.5 ? 4 * t * t * t : 1 - Math.pow(-2 * t + 2, 3) / 2)

🟡 Generic easing/lerp utilities inlined in hook instead of shared geometry lib

maintainability · flagged by 1 model

  • web/src/editor/useViewport.ts:10-11easeInOutCubic and lerp are generic, reusable math utilities defined inline in a gesture hook. They should live in geometry.ts (or a shared utils module) so they're discoverable, reusable, and testable alongside the other geometry helpers.

🪰 Gadfly · advisory

🟡 **Generic easing/lerp utilities inlined in hook instead of shared geometry lib** _maintainability · flagged by 1 model_ - **`web/src/editor/useViewport.ts:10-11`** — `easeInOutCubic` and `lerp` are generic, reusable math utilities defined inline in a gesture hook. They should live in `geometry.ts` (or a shared utils module) so they're discoverable, reusable, and testable alongside the other geometry helpers. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +17,4 @@
* [MIN_SCALE, MAX_SCALE]. The svg must set touch-action:none so the browser
* doesn't hijack the gestures.
*/
export function useViewport(svgRef: RefObject<SVGSVGElement | null>) {

Hook named useViewport returns only fitToRect, not viewport state — misleading name

maintainability · flagged by 1 model

  • web/src/editor/useViewport.ts:20 — the hook is named useViewport but returns only { fitToRect } (line 92); the viewport state itself lives in the zustand store (useEditorStore). A reader expecting useViewport() to surface viewport state will be surprised. Consider useViewportGestures or have it also return the viewport. Minor naming smell.

🪰 Gadfly · advisory

⚪ **Hook named `useViewport` returns only `fitToRect`, not viewport state — misleading name** _maintainability · flagged by 1 model_ - **`web/src/editor/useViewport.ts:20`** — the hook is named `useViewport` but returns only `{ fitToRect }` (line 92); the viewport state itself lives in the zustand store (`useEditorStore`). A reader expecting `useViewport()` to surface viewport state will be surprised. Consider `useViewportGestures` or have it also return the viewport. Minor naming smell. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +28,4 @@
}
}, [])
const toLocal = useCallback(

🟠 getBoundingClientRect forces synchronous layout on every pinch/wheel frame

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

  • Forced synchronous layout in gesture hot pathweb/src/editor/useViewport.ts:33 The toLocal helper calls svgRef.current?.getBoundingClientRect() inside every onWheel and onPinch event. During a pinch gesture this fires on every touch-move frame, forcing the browser to synchronously recalculate layout (reflow) while handling input. On lower-end devices this produces jank. Fix: cache the SVG bounding offset (e.g. in a ref updated on mount/resize via ResizeObserver or the exi…

🪰 Gadfly · advisory

🟠 **getBoundingClientRect forces synchronous layout on every pinch/wheel frame** _error-handling, maintainability, performance · flagged by 2 models_ * **Forced synchronous layout in gesture hot path** — `web/src/editor/useViewport.ts:33` The `toLocal` helper calls `svgRef.current?.getBoundingClientRect()` inside every `onWheel` and `onPinch` event. During a pinch gesture this fires on every touch-move frame, forcing the browser to synchronously recalculate layout (reflow) while handling input. On lower-end devices this produces jank. **Fix:** cache the SVG bounding offset (e.g. in a ref updated on mount/resize via `ResizeObserver` or the exi… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +50,4 @@
onWheel: ({ event, delta: [, dy] }) => {
event.preventDefault()
const p = toLocal(event.clientX, event.clientY)
setViewport((vp) => zoomViewportAt(vp, p, vp.scale * Math.exp(-dy * WHEEL_SENSITIVITY), MIN_SCALE, MAX_SCALE))

🟠 onWheel does not guard against NaN dy, permanently corrupting viewport

error-handling · flagged by 1 model

  • web/src/editor/store.ts:24setViewport accepts any Viewport without clamping scale to [MIN_SCALE, MAX_SCALE]. A direct call with scale: 0 (or NaN/negative) corrupts the canvas permanently because downstream screenToWorld divides by vp.scale, yielding Infinity/NaN. The store should enforce the invariant it exports, either by clamping inside setViewport or by rejecting non-finite values. - web/src/editor/useViewport.ts:33-34toLocal silently falls back to…

🪰 Gadfly · advisory

🟠 **onWheel does not guard against NaN dy, permanently corrupting viewport** _error-handling · flagged by 1 model_ - **`web/src/editor/store.ts:24`** — `setViewport` accepts any `Viewport` without clamping `scale` to `[MIN_SCALE, MAX_SCALE]`. A direct call with `scale: 0` (or `NaN`/negative) corrupts the canvas permanently because downstream `screenToWorld` divides by `vp.scale`, yielding `Infinity`/`NaN`. The store should enforce the invariant it exports, either by clamping inside `setViewport` or by rejecting non-finite values. - **`web/src/editor/useViewport.ts:33-34`** — `toLocal` silently falls back to… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +71,4 @@
)
const fitToRect = useCallback(
(rect: Rect, viewport: Size) => {

🟡 Parameter named viewport with type Size confuses with Viewport interface

maintainability · flagged by 3 models

  • web/src/editor/useViewport.ts:74 / web/src/lib/geometry.ts:97 — The fitToRect callback and zoomToFitRect both name their second parameter viewport even though its type is Size (pixel dimensions of the container). This shadows the Viewport transform interface and is confusing when reading call sites. Rename to containerSize or viewSize.

🪰 Gadfly · advisory

🟡 **Parameter named viewport with type Size confuses with Viewport interface** _maintainability · flagged by 3 models_ - **`web/src/editor/useViewport.ts:74`** / **`web/src/lib/geometry.ts:97`** — The `fitToRect` callback and `zoomToFitRect` both name their second parameter `viewport` even though its type is `Size` (pixel dimensions of the container). This shadows the `Viewport` transform interface and is confusing when reading call sites. Rename to `containerSize` or `viewSize`. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +33,4 @@
}
export function screenToWorld(p: Point, vp: Viewport): Point {
return { x: (p.x - vp.tx) / vp.scale, y: (p.y - vp.ty) / vp.scale }

🟠 screenToWorld divides by scale without zero guard, and store allows arbitrary scale values

error-handling · flagged by 1 model

  • web/src/lib/geometry.ts:36screenToWorld divides by vp.scale without guarding against zero. The useEditorStore introduced in this PR accepts any Viewport via setViewport with no validation, so a caller (now or in a future PR) could set scale: 0, producing Infinity/NaN that propagates through transforms and breaks rendering. Fix: either add an assert(scale !== 0) or return a safe fallback, and consider clamping inside setViewport at the store level.

🪰 Gadfly · advisory

🟠 **screenToWorld divides by scale without zero guard, and store allows arbitrary scale values** _error-handling · flagged by 1 model_ - **`web/src/lib/geometry.ts:36`** — `screenToWorld` divides by `vp.scale` without guarding against zero. The `useEditorStore` introduced in this PR accepts any `Viewport` via `setViewport` with no validation, so a caller (now or in a future PR) could set `scale: 0`, producing `Infinity`/`NaN` that propagates through transforms and breaks rendering. **Fix:** either add an `assert(scale !== 0)` or return a safe fallback, and consider clamping inside `setViewport` at the store level. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +94,4 @@
* with `padding` px of margin on every side. Scale is clamped to [minScale,
* maxScale] so a tiny/huge rect still lands in a sane zoom.
*/
export function zoomToFitRect(

🟡 Parameter named viewport with type Size confuses with Viewport interface

maintainability · flagged by 1 model

  • web/src/editor/useViewport.ts:74 / web/src/lib/geometry.ts:97 — The fitToRect callback and zoomToFitRect both name their second parameter viewport even though its type is Size (pixel dimensions of the container). This shadows the Viewport transform interface and is confusing when reading call sites. Rename to containerSize or viewSize.

🪰 Gadfly · advisory

🟡 **Parameter named viewport with type Size confuses with Viewport interface** _maintainability · flagged by 1 model_ - **`web/src/editor/useViewport.ts:74`** / **`web/src/lib/geometry.ts:97`** — The `fitToRect` callback and `zoomToFitRect` both name their second parameter `viewport` even though its type is `Size` (pixel dimensions of the container). This shadows the `Viewport` transform interface and is confusing when reading call sites. Rename to `containerSize` or `viewSize`. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +101,4 @@
minScale: number,
maxScale: number,
): Viewport {
const availW = Math.max(1, viewport.w - padding * 2)

🟠 zoomToFitRect mishandles negative rect dimensions, mis-centering the viewport

error-handling · flagged by 1 model

  • web/src/editor/store.ts:24setViewport accepts any Viewport without clamping scale to [MIN_SCALE, MAX_SCALE]. A direct call with scale: 0 (or NaN/negative) corrupts the canvas permanently because downstream screenToWorld divides by vp.scale, yielding Infinity/NaN. The store should enforce the invariant it exports, either by clamping inside setViewport or by rejecting non-finite values. - web/src/editor/useViewport.ts:33-34toLocal silently falls back to…

🪰 Gadfly · advisory

🟠 **zoomToFitRect mishandles negative rect dimensions, mis-centering the viewport** _error-handling · flagged by 1 model_ - **`web/src/editor/store.ts:24`** — `setViewport` accepts any `Viewport` without clamping `scale` to `[MIN_SCALE, MAX_SCALE]`. A direct call with `scale: 0` (or `NaN`/negative) corrupts the canvas permanently because downstream `screenToWorld` divides by `vp.scale`, yielding `Infinity`/`NaN`. The store should enforce the invariant it exports, either by clamping inside `setViewport` or by rejecting non-finite values. - **`web/src/editor/useViewport.ts:33-34`** — `toLocal` silently falls back to… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Minor issues · 22 findings (5 with multi-model agreement)

Finding Where Models Lens
🟠 ObjectShape renders invalid SVG for zero or negative dimensions web/src/editor/ObjectShape.tsx:37 3/5 error-handling, maintainability
🟡 Parameter named viewport with type Size confuses with Viewport interface web/src/editor/useViewport.ts:74 3/5 maintainability
🟠 Hardcoded SVG pattern ID risks collision across canvas instances web/src/editor/GardenCanvas.tsx:69 2/5 error-handling, maintainability
🟠 getBoundingClientRect forces synchronous layout on every pinch/wheel frame web/src/editor/useViewport.ts:31 2/5 error-handling, maintainability, performance
🟡 ObjectShape lacks memoization, causing unnecessary re-render of every object on each pan/zoom/pinch/fit-animation frame web/src/editor/ObjectShape.tsx:27 2/5 performance
17 single-model findings (lower confidence)
Finding Where Model Lens
🟠 fittedRef prevents re-fit when garden prop changes web/src/editor/GardenCanvas.tsx:21 opencode/kimi-k2.6:cloud maintainability
🟠 Auto-fit effect lacks garden dimension validation, allowing NaN viewport web/src/editor/GardenCanvas.tsx:46 opencode/kimi-k2.6:cloud error-handling
🟠 Circle shapes render radius from widthCm only, silently ignoring heightCm even though the backend allows them to differ web/src/editor/ObjectShape.tsx:60 claude-code/sonnet correctness
🟠 setViewport accepts unclamped scale, allowing division-by-zero corruption web/src/editor/store.ts:24 opencode/kimi-k2.6:cloud error-handling
🟠 onWheel does not guard against NaN dy, permanently corrupting viewport web/src/editor/useViewport.ts:53 opencode/kimi-k2.6:cloud error-handling
🟠 screenToWorld divides by scale without zero guard, and store allows arbitrary scale values web/src/lib/geometry.ts:36 kimi-k2.6:cloud error-handling
🟠 zoomToFitRect mishandles negative rect dimensions, mis-centering the viewport web/src/lib/geometry.ts:104 opencode/kimi-k2.6:cloud error-handling
🟡 Fit button duplicates Button/buttonClasses styling instead of reusing the shared component web/src/editor/GardenCanvas.tsx:111 claude-code/sonnet maintainability
🟡 focusedObjectId/setFocusedObject declared but unused (dead state until #15) web/src/editor/store.ts:18 glm-5.2:cloud maintainability
🟡 EditorObject.plantable is carried in mock data but never read by the canvas web/src/editor/types.ts:18 glm-5.2:cloud maintainability
🟡 Generic easing/lerp utilities inlined in hook instead of shared geometry lib web/src/editor/useViewport.ts:10 kimi-k2.6:cloud maintainability
🟡 Parameter named viewport with type Size confuses with Viewport interface web/src/lib/geometry.ts:97 kimi-k2.6:cloud maintainability
Doc says "fading 1 m grid" but grid only toggles with constant opacity (no fade) web/src/editor/GardenCanvas.tsx:13 opencode/glm-5.2:cloud maintainability
Duplicated #8a8a8a default fill (kindColors entry + fillFor fallback) web/src/editor/ObjectShape.tsx:6 glm-5.2:cloud maintainability
Uncommented magic factors 0.06 (rx) and 0.28 (font) lack rationale web/src/editor/ObjectShape.tsx:70 glm-5.2:cloud maintainability
EditorGarden.unitPref re-declares the UnitPref union instead of importing it from lib/units.ts web/src/editor/types.ts:27 claude-code/sonnet maintainability
Hook named 'useViewport' returns only 'fitToRect', not viewport state — misleading name web/src/editor/useViewport.ts:20 opencode/glm-5.2:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

This matches the diff already committed on this branch — the changes are just dependency/lockfile additions (@use-gesture/react, zustand, vitest + transitive lockfile entries) with no logic, config, or code changes to review. There's nothing here that could produce a substantive finding, consistent with the draft's empty findings list.

Note: this task is a read-only code-review verification request, not a software-engineering task requiring a plan — so I'm answering directly per the verification-gate instructions rather than drafting a plan file.

VERDICT: No material issues found

🎯 Correctness — Minor issues

VERDICT: Minor issues

Confirmed by reading code:

  • web/src/editor/ObjectShape.tsx:60 — circle rendering uses only halfW (r={halfW}) for the radius and silently ignores heightCm. Cross-checked internal/service/objects.go:223-256 (finalizeObject): it validates WidthCM and HeightCM independently via validDimensionCM with no constraint that they be equal for ShapeCircle objects. So a backend-valid object with shape=circle, widthCm=100, heightCm=140 would render as a perfect circle of radius 50, silently discarding the height. Also confirmed the current mock data (web/src/pages/GardenEditorPage.tsx:11,13) always sets equal width/height for its two circle objects (120/120 and 200/200), which is why this doesn't surface today. ObjectShape.tsx is new in this PR (commit 30b36b7), so this is a real gap introduced here.

Verified as correct (re-derived from first principles, not just trusted):

  • localToWorld/worldToLocal in web/src/lib/geometry.ts:46-64 — re-derived the rotation matrix and its inverse by hand; rotating local (10,0) by 90° lands at center + (0,10), confirming the clockwise-on-y-down-screen convention, consistent with the SVG rotate() transform used in ObjectShape.tsx:49.
  • zoomViewportAt/zoomToFitRect/clampScale (web/src/lib/geometry.ts) — algebra checks out against cursor-anchored zoom and centered fit-to-rect invariants.
  • web/src/lib/units.ts conversions — hand-verified against units.test.ts expected values.

No other logic bugs found in the viewport gesture wiring, store, or canvas component.

🧹 Code cleanliness & maintainability — Minor issues

Both findings are confirmed against the actual source. The Fit button's className string exactly matches what buttonClasses('ghost') in Button.tsx:7-17 would produce (base classes + ghost variant classes), and EditorGarden.unitPref does redeclare the literal union that UnitPref (units.ts:5) already exports and that gardens.ts:7,40 imports for the same purpose.

VERDICT: Minor issues

Scoped to code cleanliness/maintainability only (correctness, security, performance are covered by other lenses).

  • web/src/editor/GardenCanvas.tsx:111 — The "Fit" button hand-rolls its own Tailwind class string instead of reusing the shared Button/buttonClasses helper (web/src/components/ui/Button.tsx:7-17), which already defines this exact "ghost" styling. The rest of the app follows the shared-button convention (GardensPage.tsx uses <Button>). This new component is the one place that breaks that pattern, so a future button-style tweak would need updating in two places. Fix: <button className={buttonClasses('ghost')}>Fit</button> (or <Button variant="ghost">), keeping absolute bottom-3 right-3 as a wrapper.

  • web/src/editor/types.ts:27EditorGarden.unitPref is typed as the inline literal 'metric' | 'imperial' instead of reusing the existing UnitPref type exported from web/src/lib/units.ts:5, which web/src/lib/gardens.ts:7,40 already imports for the same purpose. Duplicated source of truth for a domain-relevant union. Fix: import type { UnitPref } from '@/lib/units' and use it in place of the inline literal.

Performance — Minor issues

Verified the finding directly against source: GardenCanvas.tsx subscribes to viewport from the zustand store and passes freshly-created <ObjectShape> elements from ordered.map(...) on every render (lines 23, 97-99); ObjectShape.tsx has no memo() wrap (confirmed via repo-wide grep — no memo( calls anywhere in web/src/editor); useViewport.ts calls setViewport from onDrag/onWheel/onPinch handlers and from a requestAnimationFrame loop in fitToRect (~21 frames over the 350ms FIT_DURATION_MS), each of which changes the viewport object identity in the store. This reproduces the draft's reasoning exactly: every gesture tick and every fit-animation frame re-renders GardenCanvas, and without memoization every ObjectShape instance is reconciled even though only the parent <g transform> changed. The finding holds.

VERDICT: Minor issues

  • web/src/editor/GardenCanvas.tsx:97-99 / web/src/editor/ObjectShape.tsx:27ObjectShape isn't memoized, so every object in the scene re-renders on every pan/wheel/pinch tick and on every frame of the fitToRect animation tween. GardenCanvas subscribes to viewport from the zustand store (GardenCanvas.tsx:23), and useViewport's onDrag/onWheel/onPinch handlers call setViewport on essentially every gesture event (useViewport.ts:42-58), while fitToRect's rAF loop calls it on ~21 frames over 350ms (useViewport.ts:79-85). Each of those state updates re-renders GardenCanvas, and because ordered.map(...) creates new ObjectShape elements every render with no React.memo on the component, React re-invokes and reconciles every object in the garden on every one of those high-frequency events — even though only the parent <g transform> actually changed and each object's own props are unchanged. With the current mock scene this is invisible, but the PR explicitly frames this as the foundation other phases (real data, plops) build on top of, and plops will live nested inside these same object shapes, compounding the per-frame reconciliation cost during pan/zoom. Wrapping ObjectShape in React.memo (its props — object, selected, onSelect — are all stable/referentially-equal across these gesture-driven re-renders) would eliminate the wasted work.
🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

I reviewed lib/geometry.ts, editor/useViewport.ts, editor/GardenCanvas.tsx, editor/ObjectShape.tsx, and editor/store.ts specifically for swallowed errors, missing cleanup, and edge/boundary conditions (nil/zero/negative/off-by-one).

What I checked and found handled correctly:

  • Division-by-zero / NaN scale propagation (geometry.ts:36 screenToWorld divides by vp.scale): every path that produces a new Viewport (zoomViewportAt, zoomToFitRect, the fitToRect rAF lerp) routes the resulting scale through clampScale(…, MIN_SCALE, MAX_SCALE) with MIN_SCALE = 0.05, so scale can never reach 0. Verified by reading store.ts (initial scale: 1) and all three call sites in useViewport.ts.
  • Degenerate fit rect (geometry.ts:104-107): zoomToFitRect guards availW/availH with Math.max(1, …) and rectW/rectH with Math.max(rect.w, 1e-6), so a zero-size viewport or a zero-size garden can't produce Infinity/NaN/division-by-zero — confirmed against the "respects the min/max scale clamp" test case (geometry.test.ts:91-93).
  • rAF/animation cleanup: useViewport.ts:90 cancels any in-flight fitToRect tween on unmount, and every gesture start (onDragStart, onWheelStart, onPinchStart) calls cancelAnim before applying a new transform, so an in-progress animated fit can't fight a user gesture.
  • ResizeObserver cleanup: GardenCanvas.tsx:37-41 disconnects the observer on unmount.
  • toLocal fallback (useViewport.ts:31-37): if svgRef.current is momentarily null, it falls back to raw client coordinates rather than throwing — a safe (if imprecise) degradation, not a crash.

One trivial, non-blocking observation: the Fit button (GardenCanvas.tsx:110) only guards size.w > 0, not size.h > 0, before calling fitToRect. In principle a 0-height/nonzero-width container could trigger a fit with a near-degenerate scale for one frame, but zoomToFitRect's Math.max(1, …) clamp already prevents any crash or NaN, so this doesn't rise to a real defect — not including it as a finding.

glm-5.2:cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

Verdict: No material issues found

Through the security lens, this change is a client-side SVG canvas rendering mock data with no network calls, auth, secrets, or injection sinks:

  • No XSS sinks: Object fields are rendered either as React text children (<text>{object.name}</text>) or as JSX attribute values (fill={fill}, transform={...}, strokeWidth, rx). React escapes text content and stringifies/escapes attribute values, so neither object.name, object.color, nor the numeric geometry fields can inject markup or break out of the attribute context. There is no dangerouslySetInnerHTML, innerHTML, eval, or dynamic <a href>/xlink:href anywhere in the new files.
  • No resource/SSF vectors: fill={o.color ?? kindColors[o.kind]} accepts an arbitrary string, but SVG fill does not execute script (no javascript: URIs are honored for fill), and the value is only used as a paint attribute — at worst a malformed color renders nothing. No external fetch is driven by these values.
  • No untrusted input yet: GardenEditorPage is wired to hardcoded mock objects; #11 will map server /full data onto the typed EditorObject. The typed surface (numbers + strings rendered via React) is the relevant taint boundary, and it's safe by construction as written.
  • No secrets/credentials introduced; new deps (zustand, @use-gesture/react, vitest) are standard, pinned in the lockfile, and carry no config of concern.

I verified the above by reading ObjectShape.tsx (the only place object data reaches DOM), GardenCanvas.tsx, useViewport.ts, store.ts, and GardenEditorPage.tsx.

🎯 Correctness — No material issues found

I've verified all the geometry math, transforms, gesture wiring, and cross-file references against the actual checked-out code. Everything checks out from a correctness perspective.

Verdict: No material issues found

Through the correctness lens, I verified the following by reading the actual code:

  • geometry.ts transformsworldToScreen/screenToWorld are exact inverses; localToWorld/worldToLocal are correct inverses (transposed rotation matrix). The 90° clockwise-on-screen (y-down) rotation was re-derived: for (10,0) at 90°, the result is (center.x, center.y + 10) — 10cm below center — which matches the documented clockwise convention and the test. ✓
  • zoomViewportAt — Re-derived the cursor-stationary property: screen = world·scale + t, keeping world fixed under screenPoint gives tx = p.x − world.x·scale. Correct; verified against the passing test. ✓
  • zoomToFitRect — Re-derived the 1000×1000 rect, 800×600 viewport, padding 20 case: limiting dim is height → (600−40)/1000 = 0.56; center (500,500) maps to (400,300). Matches code and test exactly. Min/max clamp path correct. ✓
  • useViewport.ts wheeldelta: [, dy] correctly indexes the y-axis delta; the Math.exp(-dy * SENSITIVITY) exponential zoom with negative sign (wheel-up = zoom in) is the standard, correct direction. toLocal correctly converts client coords to svg-local via getBoundingClientRect. ✓
  • useViewport.ts pinchfrom seeds offset[0] with current scale so offset: [scale] is absolute, passed straight into zoomViewportAt. ✓
  • fitToRect tweencancelAnim before starting, lerp over eased t, sets animRef.current = null at t=1 (no stray rAF). ✓
  • GardenCanvas first-fitfittedRef gates a one-shot fit once size is known; gardenRect memo depends on the right fields; sort by zIndex ascending. ✓
  • GardenEditorPagegetRouteApi/PageStub imports fully removed; no stale gardenId usage; mock data conforms to EditorObject/EditorGarden (verified the type fields in types.ts). ✓

No semantic, formula, constant, or unit errors in my lane. The units.test.ts cases (4ft→122cm, 4'6"→137cm, imperial rounding) also re-derive correctly from units.ts.

🧹 Code cleanliness & maintainability — Minor issues

All five findings verified against the actual code:

  1. focusedObjectId/setFocusedObject — grep confirms only the store's own declaration/definition; no other references.
  2. plantable — grep confirms only types.ts declaration + GardenEditorPage.tsx mock assignments; never read.
  3. toLocal — confirmed returns clientX - rect.left (screen-local); geometry.ts uses localToWorld/worldToLocal for object-local frame.
  4. #8a8a8a — confirmed at ObjectShape.tsx:13 (structure color) and :17 (fillFor fallback).
  5. Magic factors 0.06 (:70) and 0.28 (:40) — confirmed; the font line has a vague "sensible cm range" comment but the factor itself is unexplained.

Verdict: Minor issues

Through the code-cleanliness & maintainability lens, the change is generally well-structured and follows the surrounding patterns (token-based Tailwind classes match the existing bg-surface/text-muted/border-border conventions). A few small maintainability nits:

  • editor/store.ts:18-19,30-31focusedObjectId and setFocusedObject are declared but never read or called anywhere in the PR. Verified via repo-wide grep: the only references are the store's own declaration/definition (the comment claims "#15", but until then it's dead state that adds surface area to the store interface for no current behavior). Either drop them until #15 or keep them, but they currently read as speculative API. Minor.
  • editor/types.ts:18EditorObject.plantable is carried on every mock object but never consumed by the canvas/ObjectShape (verified by grep — only assigned in mock data, never read). Same "future use" smell; harmless but worth a note so it doesn't get copy-pasted into the #10/#11 mapping without an actual consumer.
  • editor/useViewport.ts:31toLocal returns screen-local coordinates (clientX - rect.left), but geometry.ts uses "local" to mean an object's local frame (worldToLocal/localToWorld). Two distinct meanings of "local" in adjacent modules is a readability trap. Consider renaming to toSvgPoint/clientToSvg for clarity.
  • editor/ObjectShape.tsx:6-14,17#8a8a8a appears as both the structure kind color and the fillFor fallback default. The duplicated magic default is trivial, but a single DEFAULT_FILL constant would make the fallback intent explicit.
  • editor/ObjectShape.tsx:70,40* 0.06 (corner radius) and * 0.28 (label font) are uncommented magic numbers; the font one has a vague "sensible cm range" comment but the factor itself is unexplained. A one-line rationale would help future maintainers tune them.

Nothing here is blocking; the dead-store and naming items are the most worth addressing.

Performance — Minor issues

Verdict: Minor issues

  • ObjectShape is not memoized, so every viewport update re-renders all objects. Confirmed: GardenCanvas subscribes to viewport (web/src/editor/GardenCanvas.tsx:23) and thus re-renders on each pan delta, wheel tick, pinch step, and fit-animation frame; the memoized ordered array (GardenCanvas.tsx:53) keeps each object reference stable across those renders, and select (store.ts:28) is a stable Zustand action reference, so during a pure pan/zoom the only thing changing is the parent's viewport — yet because ObjectShape (web/src/editor/ObjectShape.tsx:27) is a plain export function ObjectShape(...) with no React.memo, every GardenCanvas re-render re-runs each ObjectShape body and re-renders the whole object subtree through React reconciliation. The single world <g transform> already encodes the viewport at the SVG layer, so children's geometry doesn't need recomputation. Impact is small with the 6 mock objects but this is the hot path the real-data foundation (#11) builds on; React.memo(ObjectShape) would make each pan/zoom tick O(1) in React rather than O(N). Verified via store.ts (stable action refs), GardenCanvas.tsx (memoized array, stable select/object/selected props during pan/zoom), and useViewport.ts (per-event setViewport triggers viewport subscribers).
🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

Through the error-handling & edge-case lens, I verified the new code and it handles its unhappy paths well:

  • useViewport.ts:24-29 & :90 — rAF tween is cancelled via cancelAnim on every gesture start (onDragStart/onWheelStart/onPinchStart) and on unmount (useEffect(() => cancelAnim, [cancelAnim])). cancelAnim is useCallback([])-stable, so the unmount cleanup runs exactly once. No leaked animation frames. Verified by reading the file.
  • useViewport.ts:50-54 — wheel listener is registered with eventOptions: { passive: false }, so event.preventDefault() is valid (no swallowed error / console warning). Verified.
  • geometry.ts:104-108 (zoomToFitRect) — divide-by-zero is guarded: availW/availH floored at 1, rectW/rectH floored at 1e-6. A zero/negative padding or degenerate rect won't blow up. Verified.
  • geometry.ts:84-89 (zoomViewportAt)nextScale is clamped before use; screenToWorld divides by vp.scale, which the store always keeps in [0.05, 20] along the live gesture paths, so no division by zero in practice. Verified.
  • ObjectShape.tsx:40 — label fontCm clamps negative/zero dimensions to 8 via Math.max(8, Math.min(40, …)); no NaN/Infinity path from bad dimensions. Verified.
  • GardenCanvas.tsx:34-42 — ResizeObserver is created only after a null-guard on containerRef.current and is disconnected in cleanup. setSize can receive 0-sized rects (container collapsed), but the first-fit effect guards on size.w > 0 && size.h > 0. Verified.
  • GardenCanvas.tsx:53[...objects].sort(...) is safe for an empty array; objects is a required typed prop. Verified.

Trivial observation (not material): useViewport.ts:33-34 toLocal falls back to raw clientX/clientY when getBoundingClientRect() returns null (element unmounted). This is a silent fallback rather than a throw, but it's unreachable in practice — gestures can't fire on a detached svg — so it's harmless defensive code, not a bug.

kimi-k2.6:cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

No material issues found

I traced all untrusted-data paths through the new canvas code:

  • Mock data → SVG attributes: xCm, yCm, rotationDeg, widthCm, heightCm, and viewport fields (tx, ty, scale) are all typed number and flow into SVG transform, width, height, etc. The string-template construction of transform (translate(${n} ${n}) scale(${n})) is safe because the values are numeric and bounded by gesture math / clamping; there is no injection path today.
  • object.name<text> node: React escapes text children, so no markup injection.
  • object.colorfill attribute: Values come from the mock color map or the optional color field. React properly escapes attribute values, and SVG fill does not execute JavaScript in modern browsers.
  • No deserialization, no URLs, no secrets, no eval, no storage: The Zustand store is explicitly ephemeral, no persistence layer is introduced, and no network requests are made.
  • Gesture event handling: passive: false is correctly paired with event.preventDefault() in the wheel handler. touch-action: none is intentional and required for custom gestures.

The surface area is small and cleanly handled. Nothing in the security lens.

🎯 Correctness — No material issues found

No material issues found

I scrutinized the geometry transforms, zoom math, unit conversions, gesture wiring, and rendering logic across all changed files. Here's what I verified:

Geometry math (lib/geometry.ts): The world↔screen transforms (worldToScreen / screenToWorld) are straightforward scale-then-translate and inverse. The local↔world rotation uses the standard 2-D rotation matrix; because the coordinate system is y-down, a positive angle visually rotates clockwise, exactly as documented and confirmed by the test at geometry.test.ts:42.

Cursor-anchored zoom (zoomViewportAt): Re-derived from first principles. For a target scale s', the new translation is tx' = px - wx * s' where wx = (px - tx) / s is the world point under the cursor. Substituting gives tx' = px - (px - tx)/s * s', which keeps the world point stationary. The code matches this formula exactly.

Fit-to-rect (zoomToFitRect): Computes scale = min( (vpW-2p)/rectW, (vpH-2p)/rectH ) and centers by translating the rect's center to the viewport center. Verified algebraically against the test expectations at geometry.test.ts:78.

Unit conversions (lib/units.ts): Re-derived all constants (CM_PER_INCH = 2.54, CM_PER_METER = 100). cmFromFtIn(4, 6) = round((54)*2.54) = 137 ✓. displayFromCm(122, 'imperial') = round(122/30.48 * 10) / 10 = 4 ✓. All test assertions check out.

Gesture wiring (useViewport.ts): Wheel uses Math.exp(-dy * sensitivity) for smooth exponential zoom; drag adds delta to translate. The from: () => [currentScale, 0] pinch seeding is correct for absolute-scale origin-anchored zoom.

Rendering (ObjectShape.tsx, GardenCanvas.tsx): The SVG pattern draws top+left edges per cell, which tile correctly. vector-effect="non-scaling-stroke" keeps stroke widths constant in screen pixels. z-ordering is done by sorting on zIndex. Mock objects all fit within the 1200×800 cm garden bounds.

No semantic errors, incorrect formulas, or miscalculated constants were found.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • web/src/editor/GardenCanvas.tsx:69 — Hardcoded SVG <pattern id="garden-grid"> will collide if multiple GardenCanvas instances ever render on the same page (e.g. side-by-side comparisons, future features, tests). Use React's useId() to generate a unique pattern ID per instance and reference it with fill={url(#${gridId})}.

  • web/src/editor/useViewport.ts:74 / web/src/lib/geometry.ts:97 — The fitToRect callback and zoomToFitRect both name their second parameter viewport even though its type is Size (pixel dimensions of the container). This shadows the Viewport transform interface and is confusing when reading call sites. Rename to containerSize or viewSize.

  • web/src/editor/useViewport.ts:10-11easeInOutCubic and lerp are generic, reusable math utilities defined inline in a gesture hook. They should live in geometry.ts (or a shared utils module) so they're discoverable, reusable, and testable alongside the other geometry helpers.

  • web/src/editor/ObjectShape.tsx:42handleDown is re-created on every render. Since this component renders per object, wrapping it in useCallback avoids unnecessary event-handler churn and follows the pattern the rest of the codebase already uses (e.g. useCallback in useViewport).

Performance — No material issues found

No material issues found

🧯 Error handling & edge cases — Minor issues

Minor issues

  • web/src/editor/GardenCanvas.tsx:69 — Hardcoded id="garden-grid" on the SVG <pattern> is document-scoped. If multiple GardenCanvas instances ever render on the same page (e.g., split view, compare mode, or future feature), the ID collides and all instances reuse the first pattern definition, causing visual glitches. Fix: generate a unique ID per instance (e.g., useId() or a counter) and reference it in the rect's fill.

  • web/src/lib/geometry.ts:36screenToWorld divides by vp.scale without guarding against zero. The useEditorStore introduced in this PR accepts any Viewport via setViewport with no validation, so a caller (now or in a future PR) could set scale: 0, producing Infinity/NaN that propagates through transforms and breaks rendering. Fix: either add an assert(scale !== 0) or return a safe fallback, and consider clamping inside setViewport at the store level.

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

  • Reviewed through the security lens. The new editor canvas renders mock/own-user data behind requireAuth (router.tsx:92). No dangerouslySetInnerHTML/innerHTML anywhere in the diff; SVG <text> content (object.name) and attribute values (object.color, kind-derived fills, transform strings) flow through React's escaping and are constructed only from numeric/typed fields — no string interpolation into executable context. No SQL/command/template injection surface, no SSRF, no deserialization, no secrets. Scale/viewport math operates on numbers with clamping (clampScale, MIN_SCALE/MAX_SCALE), so no unbounded-state concern in my lane.
  • Verified by reading web/src/editor/ObjectShape.tsx, types.ts, GardenEditorPage.tsx, and grepping the route guard (web/src/router.tsx:89-94) plus dangerouslySetInnerHTML/innerHTML (none). EditorObject.color is the only externally-supplied-presentational field that will later come from the server; React renders it as a plain attribute value, which does not execute (no javascript:-URL sink for shape fill). Not a finding for this PR.
🎯 Correctness — No material issues found

I'll examine the geometry and viewport logic for correctness issues.
Now let me verify the key math. The ObjectShape circle uses r={halfW} (width/2), ignoring height. And worldToLocal/localToWorld inverses:
I have enough context. Let me now analyze the correctness carefully through my lens.

Rotation direction verification: localToWorld rotates {x:10,y:0} by 90°: cos90=0, sin90=1 → x = 500 + 10*0 - 0*1 = 500, y = 300 + 10*1 + 0*0 = 310. That's 10 below center. On a y-down screen, rotating the +x axis 90° clockwise should land pointing down (+y). ✓ Correct.

zoomViewportAt: Keeps world point under cursor stationary. world = screenToWorld(point, vp); then tx = point.x - world.xscale. New screen = worldscale + tx = worldscale + point.x - world.xscale = point.x. ✓ Correct even after clamp (since world is computed from the old vp, the new tx compensates for the clamped scale).

zoomToFitRect centering: center maps to viewport center: screen = centerscale + tx; tx = viewport.w/2 - centerscale → screen = viewport.w/2. ✓

Circle radius: Uses r={halfW} = widthCm/2. For a circle, "width" represents diameter; radius = width/2. ✓ Reasonable convention.

Grid threshold: showGrid = GRID_CM * viewport.scale >= GRID_MIN_CELL_PX = 100*scale >= 6 → scale >= 0.06. Min scale is 0.05, so at extreme min zoom grid hides. ✓ Reasonable.

Now let me look for issues:

Issue 1 — worldToLocal/localToWorld inverse: Verified by round-trip test and manually: inverse of rotation by angle θ is rotation by -θ. localToWorld uses [cosθ, -sinθ; sinθ, cosθ]. worldToLocal: [cosθ, sinθ; -sinθ, cosθ] = rotation by -θ. ✓ Correct.

Issue 2 — Wheel zoom direction: Math.exp(-dy * WHEEL_SENSITIVITY). Standard wheel: dy>0 = scroll down = zoom out. exp(-positive) < 1 → scale decreases. ✓ Correct.

Issue 3 — fitToRect clamping during animation: The animation lerps scale linearly from from.scale to to.scale (clamped target). But the intermediate scale values are NOT clamped — if from.scale exceeds bounds (shouldn't, since input is always clamped) it's fine. Actually a subtle issue: the final value at t=1 is to.scale which is clamped. But lerp(from.scale, to.scale, 1) === to.scale. ✓ Fine.

Issue 4 — fitToRect dependency & re-fit: fittedRef prevents auto-fit only once, but if garden changes (different widthCm), no re-fit. Out of scope for mock. Not a correctness bug in this PR.

Issue 5 — Drag handler pinching guard: On onDrag, checks pinching and cancels. ✓

Issue 6 — useGesture from for pinch: from: () => [scale, 0] returns array. use-gesture pinch from expects [da, db] where da is scale. ✓

Issue 7 — ObjectShape circle with non-equal width/height: Circle uses r=halfW ignoring height. If a circle object has width≠height it renders as a circle based on width only — visually inconsistent with bbox. Minor; mock data uses equal (120×120). Not a real bug for current data.

Issue 8 — text rotation: Text is inside the rotated <g>, so the label rotates with the object. For a 30° bed "Angled", the text is rotated 30°. That's intentional (label rotates with object). Not a bug.

Issue 9 — fontCm units: fontSize in cm units (world space), then scaled by viewport. Math.min(width, height)*0.28 — for a 120cm circle, fontCm = 33.6cm. At scale 0.5 px/cm → 16.8px. Reasonable. ✓

Issue 10 — screenToWorld at the cursor for wheel: uses toLocal(event.clientX, event.clientY) which subtracts svg bounding rect → svg-local pixels. worldToScreen/screenToWorld use svg-local pixel space (since the <g transform> is within svg coords). ✓ Consistent.

Let me double-check the zoomViewportAt clamping edge: when nextScale is clamped, the stationary-point property still holds because world is from the OLD vp, and the new tx/ty are derived from the clamped scale. So the world point under the cursor stays put even when clamping kicks in. ✓ Correct (and the test confirms scale clamps to 20 at extreme).

Now let me check one potential real issue: the wheel handler uses event.clientX/Y but event here — is it a WheelEvent? use-gesture's onWheel event is a WheelEvent with clientX/Y. ✓

Let me check the delta: [, dy] destructure for wheel — use-gesture wheel delta is [dx, dy] (horizontal, vertical). ✓

Let me verify the units test expectations re-derivation:

cmFromFtIn(4): (4*12+0)*2.54 = 121.92 → round 122. ✓
cmFromFtIn(4,6): (48+6)*2.54 = 137.16 → 137. ✓
formatCm(122,'imperial'): 122/2.54 = 48.0315 in; feet=4, inches=round(0.0315)=0 → "4′ 0″". ✓ But test expects "4′ 0″". ✓
formatCm(137,'imperial'): 137/2.54 = 53.937 in; feet=4, inches=round(53.937-48)=round(5.937)=6 → "4′ 6″". ✓

Now, one subtle correctness issue in formatCm imperial: inches = Math.round(totalInches - feet * INCHES_PER_FOOT). If totalInches = 47.9 (just under 4ft), feet=3, inches=round(11.9)=12 → carries to 4′ 0″. ✓ Handled.

Let me look at the displayFromCm imperial rounding — rounds to 0.1 ft. ✓

Everything in my lens checks out. Let me check the wheel event's delta sign convention once more — actually different browsers/devices vary, but Math.exp(-dy*SENS) with dy>0 (scroll down/away) zooms out, which is the standard trackpad expectation. Not a bug.

Let me verify one final potential issue — useGesture config eventOptions: { passive: false } is applied globally; with target: svgRef and React also having onPointerDown on the svg. The gesture lib binds its own listeners via target. React's onPointerDown is separate (delegated). Both fire. Object pointerdown calls stopPropagation() — but does that stop use-gesture's drag from starting? use-gesture attaches listeners to the target (svg) in capture/bubble phase via native addEventListener. React's synthetic stopPropagation stops the React synthetic event propagation but the native event continues to bubble to document where use-gesture... actually use-gesture binds to the target element directly. Hmm. This could mean dragging an object also pans the canvas. But this is interaction behavior (#11's domain), and the diff notes object dragging stops propagation. Let me not over-reach — this is borderline outside pure correctness of the new math, and it's explicitly deferred to #11.

Based on my correctness lens (math, transforms, units, formulas), everything verifies correctly.

VERDICT: No material issues found.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • web/src/editor/useViewport.ts:74fitToRect(rect: Rect, viewport: Size) names its second parameter viewport, but its type is Size ({w, h}). The codebase establishes Viewport ({tx, ty, scale}) as a core type in geometry.ts (line 25) and useViewport.ts works with that Viewport concept throughout (e.g. lines 47, 53, 76). Using viewport to mean "canvas pixel size" is a real readability trap at the call sites fitToRect(gardenRect, size) in GardenCanvas.tsx:48,110. (Note: zoomToFitRect in geometry.ts:99 reuses the same convention, so this is a pre-existing smell the new code inherits rather than introduces.) Rename to viewSize or canvasSize. Verified.

  • web/src/editor/GardenCanvas.tsx:13 — doc comment says "a fading 1 m grid", but the grid just toggles on/off via showGrid (GRID_CM * viewport.scale >= GRID_MIN_CELL_PX, line 52) with constant strokeOpacity={0.18} (line 74). There is no fade. The comment will mislead maintainers. Either implement a fade (opacity as a function of cell px) or drop "fading". Verified.

  • web/src/editor/ObjectShape.tsx:40,70 — magic numbers 0.28, 8, 40 (label sizing, line 40) and 0.06 (corner radius, line 70) only have a one-line comment on the former. Fine to keep but a named constant or a brief // 0.06 → ~6% of the shorter side would help future tuners. Minor.

  • web/src/editor/useViewport.ts:20 — the hook is named useViewport but returns only { fitToRect } (line 92); the viewport state itself lives in the zustand store (useEditorStore). A reader expecting useViewport() to surface viewport state will be surprised. Consider useViewportGestures or have it also return the viewport. Minor naming smell.

Performance — No material issues found

I'll review this through the performance lens, verifying with the repo tools.
I have enough to evaluate. Let me check the GardenEditorPage mock objects to confirm scale of data.

Verdict: No material issues found

Performance lens review of this canvas foundation slice. Mock data only, six objects. Even extrapolated to plausible garden sizes the hot paths are cheap.

Verified by reading the actual files:

  • GardenCanvas.tsx:53ordered is useMemo'd on [objects] and does a single sort(). O(n log n) once per objects change, not per render. Fine.
  • useViewport.ts:39-71 — wheel/drag/pinch handlers call setViewport with functional updates; no per-event allocations of concern beyond the trivial viewport object (three numbers). toLocal calls getBoundingClientRect() per wheel/pinch event — this forces a layout read but only on actual user input events (not a hot inner loop), and it's the standard, acceptable pattern. Acceptable.
  • useViewport.ts:73-88 — fit animation runs rAF for 350ms, lerps 3 numbers, calls setViewport per frame. Bounded duration, cancels on unmount and on any gesture start. Clean.
  • store.ts:24-25setViewport spreads {...vp} in the drag path creating one small object per drag delta. Negligible; standard Zustand usage.
  • Grid rendering uses a single SVG <pattern> fill rather than per-cell lines — good, scales with viewport size not cell count. showGrid gates it (GardenCanvas.tsx:52) so it disappears when zoomed far out.
  • ObjectShape is memo-free but only re-renders when its props change; the .map in GardenCanvas.tsx:97 keys on o.id. With small N this is fine; if N ever grows large, wrapping ObjectShape in React.memo would be worth it, but that's premature now and out of scope for this lens given current data.

No N+1, no unbounded growth, no blocking calls on hot paths, no quadratic behavior. Nothing material in the performance lane.

🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

I reviewed the diff through the error-handling / edge-case lens, reading geometry.ts, useViewport.ts, and GardenCanvas.tsx in full to verify. The unhappy paths are handled correctly:

  • Division by zero in screenToWorld (geometry.ts:35-37): impossible in practice — scale is seeded at 1 and every mutation routes through clampScale with MIN_SCALE = 0.05. Verified the store (store.ts:29) and both callers (useViewport.ts:53, 58).
  • zoomToFitRect with zero/negative viewport or rect (geometry.ts:97-117): availW/availH clamped to ≥1, rectW/rectH clamped to ≥1e-6, and scale clamped to [minScale, maxScale]. No NaN/Infinity escapes. Callers also guard size.w > 0 && size.h > 0 (GardenCanvas.tsx:46, 110).
  • Wheel overflow (useViewport.ts:53): Math.exp(-dy * WHEEL_SENSITIVITY) can tend to 0 or Infinity for extreme dy, but the resulting scale is run through clampScale → safe.
  • Fit tween lifecycle (useViewport.ts:73-90): cancelAnim runs at the start of each fitToRect (so repeat clicks don't stack rAFs) and on unmount via useEffect(() => cancelAnim, [cancelAnim]) — no orphaned rAF writing to Zustand after unmount. t is clamped to [0,1] so a backgrounded tab catching up doesn't overshoot.
  • ResizeObserver (GardenCanvas.tsx:34-42): null-element guard, proper disconnect() cleanup; the first-fit effect gates on size.w > 0 && size.h > 0 so a hidden/zero-size container won't fit to a degenerate rect. fittedRef prevents re-fitting on unrelated size changes.
  • Pinch from seeding (useViewport.ts:67): reads useEditorStore.getState().viewport.scale — always defined; no nil dereference even on first pinch.
  • toLocal (useViewport.ts:31-37): falls back to raw client coords if getBoundingClientRect() is unavailable — defensive, no crash.

The only thing I'd flag as outside strict error-handling (and therefore not a finding): ObjectShape doesn't validate widthCm/heightCm > 0, so a degenerate object would render with r=0 / negative dimensions — but that's a data-validation concern, not an unhandled error in the introduced paths, and the mock data is well-formed.

No swallowed errors, no missing cleanup, no panic-prone inputs in this change.

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

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

🔒 Security — No material issues found

No material issues found

  • I reviewed all new and changed files through the security lens. The only potential injection surface I identified is object.color interpolated into SVG fill attributes (ObjectShape.tsx:55/71), which is typed as string | null | undefined with no runtime validation. SVG fill can accept url(...) references, so an arbitrary string would be a paint-value injection vector (SSRF-like requests, tracking pixels) once #11 makes this field editable. However, in this PR the field is never populated—mock objects omit color, and the field editor that would accept user input is explicitly out of scope for #9. The vulnerability surface is latent, not live.

  • No authn/authz gaps: GardenEditorPage remains behind the existing requireAuth guard; no new unprotected routes are introduced.

  • No unsafe deserialization, command/SQL injection, path traversal, SSRF, secret leakage, or missing input validation on active code paths.

  • Dependencies (zustand, @use-gesture/react, vitest) are standard, well-maintained packages with no known security red flags in the versions added.

🎯 Correctness — No material issues found

Verdict: No material issues found

I scrutinized the geometry transforms, zoom math, unit conversions, and gesture handling. Everything checks out.

  • localToWorld / worldToLocal: The rotation matrix is the standard 2D rotation. In screen coordinates (y-down), this produces clockwise rotation for positive angles, matching the documented behavior. Verified the 90° test case round-trips correctly.
  • zoomViewportAt: The algebra tx' = screenPoint.x - world.x * scale' correctly preserves the world point under the cursor. Verified against the test case: screenToWorld(cursor, next) round-trips to the original world point.
  • zoomToFitRect: Centers the rect by mapping rectCenter to viewportCenter with tx = viewport.w/2 - centerX*scale. Verified with the test assertions.
  • Unit conversions: cmFromFtIn, cmFromMeters, displayFromCm, and formatCm all use correct constants (2.54 cm/in, 12 in/ft, 100 cm/m) and appropriate rounding. Round-trip drift is within expected quantization bounds.
  • Wheel/pinch gesture math: The exponential zoom scale * exp(-dy * sensitivity) is standard and directionally correct (scroll-up zooms in). The from seed for pinch ensures absolute scale values.

No logic bugs, incorrect formulas, or semantic domain errors found.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • web/src/editor/GardenCanvas.tsx:69 — The SVG <pattern id="garden-grid"> uses a hard-coded id. If GardenCanvas is ever rendered more than once per page (or remounted), the pattern ID will collide in the DOM. Use React.useId() and reference it via url(#${gridId}) instead.

  • web/src/editor/GardenCanvas.tsx:21,45-50fittedRef gates the auto-fit effect so it only fires once per component lifetime. If the garden prop changes (which #11 will support), the canvas silently stays framed for the old garden. Track the last fitted garden ID and reset the flag when garden.id changes, or drive fit state from a stable key.

  • web/src/editor/useViewport.ts:74 — The fitToRect callback names its second parameter viewport but its type is Size (pixel dimensions). This collides semantically with the Viewport transform type used throughout the codebase. Rename it to viewSize, containerPx, or similar to disambiguate.

Performance — Minor issues

Minor issues found

  • Forced synchronous layout in gesture hot pathweb/src/editor/useViewport.ts:33
    The toLocal helper calls svgRef.current?.getBoundingClientRect() inside every onWheel and onPinch event. During a pinch gesture this fires on every touch-move frame, forcing the browser to synchronously recalculate layout (reflow) while handling input. On lower-end devices this produces jank.
    Fix: cache the SVG bounding offset (e.g. in a ref updated on mount/resize via ResizeObserver or the existing observer in GardenCanvas) and read the cached value in toLocal instead of calling getBoundingClientRect() in the handler.
🧯 Error handling & edge cases — Minor issues

Minor issues

  • web/src/editor/store.ts:24setViewport accepts any Viewport without clamping scale to [MIN_SCALE, MAX_SCALE]. A direct call with scale: 0 (or NaN/negative) corrupts the canvas permanently because downstream screenToWorld divides by vp.scale, yielding Infinity/NaN. The store should enforce the invariant it exports, either by clamping inside setViewport or by rejecting non-finite values.
  • web/src/editor/useViewport.ts:33-34toLocal silently falls back to raw clientX/clientY when svgRef.current is null (unmount timing, ref race, or conditional rendering). This makes wheel/pinch zoom anchor to the browser viewport origin instead of the SVG, producing a jarring jump. It should return a sentinel or throw rather than lie about local coordinates.
  • web/src/lib/geometry.ts:104-108zoomToFitRect silently misbehaves on negative rect dimensions. Math.max(rect.w, 1e-6) masks the width for scale math, but centerX = rect.x + rect.w / 2 (line 110) still uses the negative original, centering the viewport on the wrong point. Should validate rect.w > 0 && rect.h > 0 (or take absolute values) and error/warn instead of producing a misleading viewport.
  • web/src/editor/ObjectShape.tsx:37-40 — No guard against zero or negative widthCm/heightCm. A zero-width circle gets r={0} (invisible); a negative-width rect flips its x origin and yields a negative rx, which SVG treats as an error. The component should clamp dimensions to > 0 or skip rendering invalid geometry.
  • web/src/editor/useViewport.ts:53onWheel does not guard against NaN in dy. A synthetic or broken wheel event with dy = NaN propagates NaN through Math.expclampScalezoomViewportAt, permanently poisoning the viewport state. A Number.isFinite(dy) check (and on the resulting scale) would prevent this.
  • web/src/editor/GardenCanvas.tsx:46-49 — The auto-fit useEffect only checks canvas size positivity, not gardenRect validity. If garden.widthCm is NaN (malformed mock or future server data), zoomToFitRect returns a NaN viewport and the canvas becomes unrecoverably blank. Should guard with Number.isFinite(gardenRect.w) && gardenRect.w > 0 (same for h).

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: Minor issues** · 22 findings (5 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟠 | ObjectShape renders invalid SVG for zero or negative dimensions | `web/src/editor/ObjectShape.tsx:37` | 3/5 | error-handling, maintainability | | 🟡 | Parameter named viewport with type Size confuses with Viewport interface | `web/src/editor/useViewport.ts:74` | 3/5 | maintainability | | 🟠 | Hardcoded SVG pattern ID risks collision across canvas instances | `web/src/editor/GardenCanvas.tsx:69` | 2/5 | error-handling, maintainability | | 🟠 | getBoundingClientRect forces synchronous layout on every pinch/wheel frame | `web/src/editor/useViewport.ts:31` | 2/5 | error-handling, maintainability, performance | | 🟡 | ObjectShape lacks memoization, causing unnecessary re-render of every object on each pan/zoom/pinch/fit-animation frame | `web/src/editor/ObjectShape.tsx:27` | 2/5 | performance | <details><summary>17 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | fittedRef prevents re-fit when garden prop changes | `web/src/editor/GardenCanvas.tsx:21` | opencode/kimi-k2.6:cloud | maintainability | | 🟠 | Auto-fit effect lacks garden dimension validation, allowing NaN viewport | `web/src/editor/GardenCanvas.tsx:46` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | Circle shapes render radius from widthCm only, silently ignoring heightCm even though the backend allows them to differ | `web/src/editor/ObjectShape.tsx:60` | claude-code/sonnet | correctness | | 🟠 | setViewport accepts unclamped scale, allowing division-by-zero corruption | `web/src/editor/store.ts:24` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | onWheel does not guard against NaN dy, permanently corrupting viewport | `web/src/editor/useViewport.ts:53` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | screenToWorld divides by scale without zero guard, and store allows arbitrary scale values | `web/src/lib/geometry.ts:36` | kimi-k2.6:cloud | error-handling | | 🟠 | zoomToFitRect mishandles negative rect dimensions, mis-centering the viewport | `web/src/lib/geometry.ts:104` | opencode/kimi-k2.6:cloud | error-handling | | 🟡 | Fit button duplicates Button/buttonClasses styling instead of reusing the shared component | `web/src/editor/GardenCanvas.tsx:111` | claude-code/sonnet | maintainability | | 🟡 | focusedObjectId/setFocusedObject declared but unused (dead state until #15) | `web/src/editor/store.ts:18` | glm-5.2:cloud | maintainability | | 🟡 | EditorObject.plantable is carried in mock data but never read by the canvas | `web/src/editor/types.ts:18` | glm-5.2:cloud | maintainability | | 🟡 | Generic easing/lerp utilities inlined in hook instead of shared geometry lib | `web/src/editor/useViewport.ts:10` | kimi-k2.6:cloud | maintainability | | 🟡 | Parameter named viewport with type Size confuses with Viewport interface | `web/src/lib/geometry.ts:97` | kimi-k2.6:cloud | maintainability | | ⚪ | Doc says "fading 1 m grid" but grid only toggles with constant opacity (no fade) | `web/src/editor/GardenCanvas.tsx:13` | opencode/glm-5.2:cloud | maintainability | | ⚪ | Duplicated #8a8a8a default fill (kindColors entry + fillFor fallback) | `web/src/editor/ObjectShape.tsx:6` | glm-5.2:cloud | maintainability | | ⚪ | Uncommented magic factors 0.06 (rx) and 0.28 (font) lack rationale | `web/src/editor/ObjectShape.tsx:70` | glm-5.2:cloud | maintainability | | ⚪ | EditorGarden.unitPref re-declares the UnitPref union instead of importing it from lib/units.ts | `web/src/editor/types.ts:27` | claude-code/sonnet | maintainability | | ⚪ | Hook named 'useViewport' returns only 'fitToRect', not viewport state — misleading name | `web/src/editor/useViewport.ts:20` | opencode/glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> This matches the diff already committed on this branch — the changes are just dependency/lockfile additions (`@use-gesture/react`, `zustand`, `vitest` + transitive lockfile entries) with no logic, config, or code changes to review. There's nothing here that could produce a substantive finding, consistent with the draft's empty findings list. Note: this task is a read-only code-review verification request, not a software-engineering task requiring a plan — so I'm answering directly per the verification-gate instructions rather than drafting a plan file. VERDICT: No material issues found </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> ## VERDICT: Minor issues **Confirmed by reading code:** - `web/src/editor/ObjectShape.tsx:60` — circle rendering uses only `halfW` (`r={halfW}`) for the radius and silently ignores `heightCm`. Cross-checked `internal/service/objects.go:223-256` (`finalizeObject`): it validates `WidthCM` and `HeightCM` independently via `validDimensionCM` with no constraint that they be equal for `ShapeCircle` objects. So a backend-valid object with `shape=circle, widthCm=100, heightCm=140` would render as a perfect circle of radius 50, silently discarding the height. Also confirmed the current mock data (`web/src/pages/GardenEditorPage.tsx:11,13`) always sets equal width/height for its two circle objects (120/120 and 200/200), which is why this doesn't surface today. `ObjectShape.tsx` is new in this PR (commit 30b36b7), so this is a real gap introduced here. **Verified as correct (re-derived from first principles, not just trusted):** - `localToWorld`/`worldToLocal` in `web/src/lib/geometry.ts:46-64` — re-derived the rotation matrix and its inverse by hand; rotating local `(10,0)` by 90° lands at `center + (0,10)`, confirming the clockwise-on-y-down-screen convention, consistent with the SVG `rotate()` transform used in `ObjectShape.tsx:49`. - `zoomViewportAt`/`zoomToFitRect`/`clampScale` (`web/src/lib/geometry.ts`) — algebra checks out against cursor-anchored zoom and centered fit-to-rect invariants. - `web/src/lib/units.ts` conversions — hand-verified against `units.test.ts` expected values. No other logic bugs found in the viewport gesture wiring, store, or canvas component. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Both findings are confirmed against the actual source. The Fit button's className string exactly matches what `buttonClasses('ghost')` in `Button.tsx:7-17` would produce (base classes + ghost variant classes), and `EditorGarden.unitPref` does redeclare the literal union that `UnitPref` (units.ts:5) already exports and that `gardens.ts:7,40` imports for the same purpose. **VERDICT: Minor issues** Scoped to code cleanliness/maintainability only (correctness, security, performance are covered by other lenses). - `web/src/editor/GardenCanvas.tsx:111` — The "Fit" button hand-rolls its own Tailwind class string instead of reusing the shared `Button`/`buttonClasses` helper (`web/src/components/ui/Button.tsx:7-17`), which already defines this exact "ghost" styling. The rest of the app follows the shared-button convention (`GardensPage.tsx` uses `<Button>`). This new component is the one place that breaks that pattern, so a future button-style tweak would need updating in two places. Fix: `<button className={buttonClasses('ghost')}>Fit</button>` (or `<Button variant="ghost">`), keeping `absolute bottom-3 right-3` as a wrapper. - `web/src/editor/types.ts:27` — `EditorGarden.unitPref` is typed as the inline literal `'metric' | 'imperial'` instead of reusing the existing `UnitPref` type exported from `web/src/lib/units.ts:5`, which `web/src/lib/gardens.ts:7,40` already imports for the same purpose. Duplicated source of truth for a domain-relevant union. Fix: `import type { UnitPref } from '@/lib/units'` and use it in place of the inline literal. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> Verified the finding directly against source: `GardenCanvas.tsx` subscribes to `viewport` from the zustand store and passes freshly-created `<ObjectShape>` elements from `ordered.map(...)` on every render (lines 23, 97-99); `ObjectShape.tsx` has no `memo()` wrap (confirmed via repo-wide grep — no `memo(` calls anywhere in `web/src/editor`); `useViewport.ts` calls `setViewport` from `onDrag`/`onWheel`/`onPinch` handlers and from a `requestAnimationFrame` loop in `fitToRect` (~21 frames over the 350ms `FIT_DURATION_MS`), each of which changes the `viewport` object identity in the store. This reproduces the draft's reasoning exactly: every gesture tick and every fit-animation frame re-renders `GardenCanvas`, and without memoization every `ObjectShape` instance is reconciled even though only the parent `<g transform>` changed. The finding holds. **VERDICT: Minor issues** - `web/src/editor/GardenCanvas.tsx:97-99` / `web/src/editor/ObjectShape.tsx:27` — `ObjectShape` isn't memoized, so every object in the scene re-renders on every pan/wheel/pinch tick and on every frame of the `fitToRect` animation tween. `GardenCanvas` subscribes to `viewport` from the zustand store (`GardenCanvas.tsx:23`), and `useViewport`'s `onDrag`/`onWheel`/`onPinch` handlers call `setViewport` on essentially every gesture event (`useViewport.ts:42-58`), while `fitToRect`'s rAF loop calls it on ~21 frames over 350ms (`useViewport.ts:79-85`). Each of those state updates re-renders `GardenCanvas`, and because `ordered.map(...)` creates new `ObjectShape` elements every render with no `React.memo` on the component, React re-invokes and reconciles every object in the garden on every one of those high-frequency events — even though only the parent `<g transform>` actually changed and each object's own props are unchanged. With the current mock scene this is invisible, but the PR explicitly frames this as the foundation other phases (real data, plops) build on top of, and plops will live nested inside these same object shapes, compounding the per-frame reconciliation cost during pan/zoom. Wrapping `ObjectShape` in `React.memo` (its props — `object`, `selected`, `onSelect` — are all stable/referentially-equal across these gesture-driven re-renders) would eliminate the wasted work. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **VERDICT: No material issues found** I reviewed `lib/geometry.ts`, `editor/useViewport.ts`, `editor/GardenCanvas.tsx`, `editor/ObjectShape.tsx`, and `editor/store.ts` specifically for swallowed errors, missing cleanup, and edge/boundary conditions (nil/zero/negative/off-by-one). What I checked and found handled correctly: - **Division-by-zero / NaN scale propagation** (`geometry.ts:36` `screenToWorld` divides by `vp.scale`): every path that produces a new `Viewport` (`zoomViewportAt`, `zoomToFitRect`, the `fitToRect` rAF lerp) routes the resulting scale through `clampScale(…, MIN_SCALE, MAX_SCALE)` with `MIN_SCALE = 0.05`, so `scale` can never reach 0. Verified by reading `store.ts` (initial `scale: 1`) and all three call sites in `useViewport.ts`. - **Degenerate fit rect** (`geometry.ts:104-107`): `zoomToFitRect` guards `availW`/`availH` with `Math.max(1, …)` and `rectW`/`rectH` with `Math.max(rect.w, 1e-6)`, so a zero-size viewport or a zero-size garden can't produce `Infinity`/`NaN`/division-by-zero — confirmed against the "respects the min/max scale clamp" test case (`geometry.test.ts:91-93`). - **rAF/animation cleanup**: `useViewport.ts:90` cancels any in-flight `fitToRect` tween on unmount, and every gesture start (`onDragStart`, `onWheelStart`, `onPinchStart`) calls `cancelAnim` before applying a new transform, so an in-progress animated fit can't fight a user gesture. - **ResizeObserver cleanup**: `GardenCanvas.tsx:37-41` disconnects the observer on unmount. - **`toLocal` fallback** (`useViewport.ts:31-37`): if `svgRef.current` is momentarily null, it falls back to raw client coordinates rather than throwing — a safe (if imprecise) degradation, not a crash. One trivial, non-blocking observation: the Fit button (`GardenCanvas.tsx:110`) only guards `size.w > 0`, not `size.h > 0`, before calling `fitToRect`. In principle a 0-height/nonzero-width container could trigger a fit with a near-degenerate scale for one frame, but `zoomToFitRect`'s `Math.max(1, …)` clamp already prevents any crash or NaN, so this doesn't rise to a real defect — not including it as a finding. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verdict: No material issues found Through the security lens, this change is a client-side SVG canvas rendering mock data with no network calls, auth, secrets, or injection sinks: - **No XSS sinks**: Object fields are rendered either as React text children (`<text>{object.name}</text>`) or as JSX attribute values (`fill={fill}`, `transform={...}`, `strokeWidth`, `rx`). React escapes text content and stringifies/escapes attribute values, so neither `object.name`, `object.color`, nor the numeric geometry fields can inject markup or break out of the attribute context. There is no `dangerouslySetInnerHTML`, `innerHTML`, `eval`, or dynamic `<a href>`/`xlink:href` anywhere in the new files. - **No resource/SSF vectors**: `fill={o.color ?? kindColors[o.kind]}` accepts an arbitrary string, but SVG `fill` does not execute script (no `javascript:` URIs are honored for `fill`), and the value is only used as a paint attribute — at worst a malformed color renders nothing. No external fetch is driven by these values. - **No untrusted input yet**: `GardenEditorPage` is wired to hardcoded mock objects; #11 will map server `/full` data onto the typed `EditorObject`. The typed surface (numbers + strings rendered via React) is the relevant taint boundary, and it's safe by construction as written. - **No secrets/credentials** introduced; new deps (`zustand`, `@use-gesture/react`, `vitest`) are standard, pinned in the lockfile, and carry no config of concern. I verified the above by reading `ObjectShape.tsx` (the only place object data reaches DOM), `GardenCanvas.tsx`, `useViewport.ts`, `store.ts`, and `GardenEditorPage.tsx`. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I've verified all the geometry math, transforms, gesture wiring, and cross-file references against the actual checked-out code. Everything checks out from a correctness perspective. **Verdict: No material issues found** Through the correctness lens, I verified the following by reading the actual code: - **`geometry.ts` transforms** — `worldToScreen`/`screenToWorld` are exact inverses; `localToWorld`/`worldToLocal` are correct inverses (transposed rotation matrix). The 90° clockwise-on-screen (y-down) rotation was re-derived: for `(10,0)` at 90°, the result is `(center.x, center.y + 10)` — 10cm below center — which matches the documented clockwise convention and the test. ✓ - **`zoomViewportAt`** — Re-derived the cursor-stationary property: `screen = world·scale + t`, keeping `world` fixed under `screenPoint` gives `tx = p.x − world.x·scale`. Correct; verified against the passing test. ✓ - **`zoomToFitRect`** — Re-derived the 1000×1000 rect, 800×600 viewport, padding 20 case: limiting dim is height → `(600−40)/1000 = 0.56`; center (500,500) maps to (400,300). Matches code and test exactly. Min/max clamp path correct. ✓ - **`useViewport.ts` wheel** — `delta: [, dy]` correctly indexes the y-axis delta; the `Math.exp(-dy * SENSITIVITY)` exponential zoom with negative sign (wheel-up = zoom in) is the standard, correct direction. `toLocal` correctly converts client coords to svg-local via `getBoundingClientRect`. ✓ - **`useViewport.ts` pinch** — `from` seeds offset[0] with current scale so `offset: [scale]` is absolute, passed straight into `zoomViewportAt`. ✓ - **`fitToRect` tween** — `cancelAnim` before starting, `lerp` over eased `t`, sets `animRef.current = null` at t=1 (no stray rAF). ✓ - **`GardenCanvas` first-fit** — `fittedRef` gates a one-shot fit once size is known; `gardenRect` memo depends on the right fields; sort by `zIndex` ascending. ✓ - **`GardenEditorPage`** — `getRouteApi`/`PageStub` imports fully removed; no stale `gardenId` usage; mock data conforms to `EditorObject`/`EditorGarden` (verified the type fields in `types.ts`). ✓ No semantic, formula, constant, or unit errors in my lane. The `units.test.ts` cases (4ft→122cm, 4'6"→137cm, imperial rounding) also re-derive correctly from `units.ts`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All five findings verified against the actual code: 1. `focusedObjectId`/`setFocusedObject` — grep confirms only the store's own declaration/definition; no other references. 2. `plantable` — grep confirms only `types.ts` declaration + `GardenEditorPage.tsx` mock assignments; never read. 3. `toLocal` — confirmed returns `clientX - rect.left` (screen-local); `geometry.ts` uses `localToWorld`/`worldToLocal` for object-local frame. 4. `#8a8a8a` — confirmed at `ObjectShape.tsx:13` (structure color) and `:17` (fillFor fallback). 5. Magic factors `0.06` (`:70`) and `0.28` (`:40`) — confirmed; the font line has a vague "sensible cm range" comment but the factor itself is unexplained. ## Verdict: Minor issues Through the code-cleanliness & maintainability lens, the change is generally well-structured and follows the surrounding patterns (token-based Tailwind classes match the existing `bg-surface`/`text-muted`/`border-border` conventions). A few small maintainability nits: - **`editor/store.ts:18-19,30-31`** — `focusedObjectId` and `setFocusedObject` are declared but never read or called anywhere in the PR. Verified via repo-wide grep: the only references are the store's own declaration/definition (the comment claims "#15", but until then it's dead state that adds surface area to the store interface for no current behavior). Either drop them until #15 or keep them, but they currently read as speculative API. Minor. - **`editor/types.ts:18`** — `EditorObject.plantable` is carried on every mock object but never consumed by the canvas/`ObjectShape` (verified by grep — only assigned in mock data, never read). Same "future use" smell; harmless but worth a note so it doesn't get copy-pasted into the #10/#11 mapping without an actual consumer. - **`editor/useViewport.ts:31`** — `toLocal` returns **screen-local** coordinates (`clientX - rect.left`), but `geometry.ts` uses "local" to mean an object's local frame (`worldToLocal`/`localToWorld`). Two distinct meanings of "local" in adjacent modules is a readability trap. Consider renaming to `toSvgPoint`/`clientToSvg` for clarity. - **`editor/ObjectShape.tsx:6-14,17`** — `#8a8a8a` appears as both the `structure` kind color and the `fillFor` fallback default. The duplicated magic default is trivial, but a single `DEFAULT_FILL` constant would make the fallback intent explicit. - **`editor/ObjectShape.tsx:70,40`** — `* 0.06` (corner radius) and `* 0.28` (label font) are uncommented magic numbers; the font one has a vague "sensible cm range" comment but the factor itself is unexplained. A one-line rationale would help future maintainers tune them. Nothing here is blocking; the dead-store and naming items are the most worth addressing. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> ## Verdict: Minor issues - **`ObjectShape` is not memoized, so every viewport update re-renders all objects.** Confirmed: `GardenCanvas` subscribes to `viewport` (`web/src/editor/GardenCanvas.tsx:23`) and thus re-renders on each pan delta, wheel tick, pinch step, and fit-animation frame; the memoized `ordered` array (`GardenCanvas.tsx:53`) keeps each `object` reference stable across those renders, and `select` (`store.ts:28`) is a stable Zustand action reference, so during a pure pan/zoom the only thing changing is the parent's viewport — yet because `ObjectShape` (`web/src/editor/ObjectShape.tsx:27`) is a plain `export function ObjectShape(...)` with no `React.memo`, every `GardenCanvas` re-render re-runs each `ObjectShape` body and re-renders the whole object subtree through React reconciliation. The single world `<g transform>` already encodes the viewport at the SVG layer, so children's geometry doesn't need recomputation. Impact is small with the 6 mock objects but this is the hot path the real-data foundation (`#11`) builds on; `React.memo(ObjectShape)` would make each pan/zoom tick O(1) in React rather than O(N). Verified via `store.ts` (stable action refs), `GardenCanvas.tsx` (memoized array, stable `select`/`object`/`selected` props during pan/zoom), and `useViewport.ts` (per-event `setViewport` triggers `viewport` subscribers). </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict: No material issues found** Through the error-handling & edge-case lens, I verified the new code and it handles its unhappy paths well: - **`useViewport.ts:24-29` & `:90`** — rAF tween is cancelled via `cancelAnim` on every gesture start (`onDragStart`/`onWheelStart`/`onPinchStart`) and on unmount (`useEffect(() => cancelAnim, [cancelAnim])`). `cancelAnim` is `useCallback([])`-stable, so the unmount cleanup runs exactly once. No leaked animation frames. Verified by reading the file. - **`useViewport.ts:50-54`** — wheel listener is registered with `eventOptions: { passive: false }`, so `event.preventDefault()` is valid (no swallowed error / console warning). Verified. - **`geometry.ts:104-108` (`zoomToFitRect`)** — divide-by-zero is guarded: `availW`/`availH` floored at `1`, `rectW`/`rectH` floored at `1e-6`. A zero/negative `padding` or degenerate rect won't blow up. Verified. - **`geometry.ts:84-89` (`zoomViewportAt`)** — `nextScale` is clamped before use; `screenToWorld` divides by `vp.scale`, which the store always keeps in `[0.05, 20]` along the live gesture paths, so no division by zero in practice. Verified. - **`ObjectShape.tsx:40`** — label `fontCm` clamps negative/zero dimensions to `8` via `Math.max(8, Math.min(40, …))`; no NaN/Infinity path from bad dimensions. Verified. - **`GardenCanvas.tsx:34-42`** — ResizeObserver is created only after a null-guard on `containerRef.current` and is disconnected in cleanup. `setSize` can receive `0`-sized rects (container collapsed), but the first-fit effect guards on `size.w > 0 && size.h > 0`. Verified. - **`GardenCanvas.tsx:53`** — `[...objects].sort(...)` is safe for an empty array; `objects` is a required typed prop. Verified. Trivial observation (not material): `useViewport.ts:33-34` `toLocal` falls back to raw `clientX/clientY` when `getBoundingClientRect()` returns null (element unmounted). This is a silent fallback rather than a throw, but it's unreachable in practice — gestures can't fire on a detached svg — so it's harmless defensive code, not a bug. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** I traced all untrusted-data paths through the new canvas code: - **Mock data → SVG attributes**: `xCm`, `yCm`, `rotationDeg`, `widthCm`, `heightCm`, and viewport fields (`tx`, `ty`, `scale`) are all typed `number` and flow into SVG `transform`, `width`, `height`, etc. The string-template construction of `transform` (`translate(${n} ${n}) scale(${n})`) is safe because the values are numeric and bounded by gesture math / clamping; there is no injection path today. - **`object.name` → `<text>` node**: React escapes text children, so no markup injection. - **`object.color` → `fill` attribute**: Values come from the mock color map or the optional `color` field. React properly escapes attribute values, and SVG `fill` does not execute JavaScript in modern browsers. - **No deserialization, no URLs, no secrets, no eval, no storage**: The Zustand store is explicitly ephemeral, no persistence layer is introduced, and no network requests are made. - **Gesture event handling**: `passive: false` is correctly paired with `event.preventDefault()` in the wheel handler. `touch-action: none` is intentional and required for custom gestures. The surface area is small and cleanly handled. Nothing in the security lens. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I scrutinized the geometry transforms, zoom math, unit conversions, gesture wiring, and rendering logic across all changed files. Here's what I verified: **Geometry math** (`lib/geometry.ts`): The world↔screen transforms (`worldToScreen` / `screenToWorld`) are straightforward scale-then-translate and inverse. The local↔world rotation uses the standard 2-D rotation matrix; because the coordinate system is y-down, a positive angle visually rotates clockwise, exactly as documented and confirmed by the test at `geometry.test.ts:42`. **Cursor-anchored zoom** (`zoomViewportAt`): Re-derived from first principles. For a target scale `s'`, the new translation is `tx' = px - wx * s'` where `wx = (px - tx) / s` is the world point under the cursor. Substituting gives `tx' = px - (px - tx)/s * s'`, which keeps the world point stationary. The code matches this formula exactly. **Fit-to-rect** (`zoomToFitRect`): Computes `scale = min( (vpW-2p)/rectW, (vpH-2p)/rectH )` and centers by translating the rect's center to the viewport center. Verified algebraically against the test expectations at `geometry.test.ts:78`. **Unit conversions** (`lib/units.ts`): Re-derived all constants (`CM_PER_INCH = 2.54`, `CM_PER_METER = 100`). `cmFromFtIn(4, 6) = round((54)*2.54) = 137` ✓. `displayFromCm(122, 'imperial') = round(122/30.48 * 10) / 10 = 4` ✓. All test assertions check out. **Gesture wiring** (`useViewport.ts`): Wheel uses `Math.exp(-dy * sensitivity)` for smooth exponential zoom; drag adds delta to translate. The `from: () => [currentScale, 0]` pinch seeding is correct for absolute-scale origin-anchored zoom. **Rendering** (`ObjectShape.tsx`, `GardenCanvas.tsx`): The SVG pattern draws top+left edges per cell, which tile correctly. `vector-effect="non-scaling-stroke"` keeps stroke widths constant in screen pixels. z-ordering is done by sorting on `zIndex`. Mock objects all fit within the 1200×800 cm garden bounds. No semantic errors, incorrect formulas, or miscalculated constants were found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`web/src/editor/GardenCanvas.tsx:69`** — Hardcoded SVG `<pattern id="garden-grid">` will collide if multiple `GardenCanvas` instances ever render on the same page (e.g. side-by-side comparisons, future features, tests). Use React's `useId()` to generate a unique pattern ID per instance and reference it with `fill={`url(#${gridId})`}`. - **`web/src/editor/useViewport.ts:74`** / **`web/src/lib/geometry.ts:97`** — The `fitToRect` callback and `zoomToFitRect` both name their second parameter `viewport` even though its type is `Size` (pixel dimensions of the container). This shadows the `Viewport` transform interface and is confusing when reading call sites. Rename to `containerSize` or `viewSize`. - **`web/src/editor/useViewport.ts:10-11`** — `easeInOutCubic` and `lerp` are generic, reusable math utilities defined inline in a gesture hook. They should live in `geometry.ts` (or a shared utils module) so they're discoverable, reusable, and testable alongside the other geometry helpers. - **`web/src/editor/ObjectShape.tsx:42`** — `handleDown` is re-created on every render. Since this component renders per object, wrapping it in `useCallback` avoids unnecessary event-handler churn and follows the pattern the rest of the codebase already uses (e.g. `useCallback` in `useViewport`). </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`web/src/editor/GardenCanvas.tsx:69`** — Hardcoded `id="garden-grid"` on the SVG `<pattern>` is document-scoped. If multiple `GardenCanvas` instances ever render on the same page (e.g., split view, compare mode, or future feature), the ID collides and all instances reuse the first pattern definition, causing visual glitches. **Fix:** generate a unique ID per instance (e.g., `useId()` or a counter) and reference it in the rect's `fill`. - **`web/src/lib/geometry.ts:36`** — `screenToWorld` divides by `vp.scale` without guarding against zero. The `useEditorStore` introduced in this PR accepts any `Viewport` via `setViewport` with no validation, so a caller (now or in a future PR) could set `scale: 0`, producing `Infinity`/`NaN` that propagates through transforms and breaks rendering. **Fix:** either add an `assert(scale !== 0)` or return a safe fallback, and consider clamping inside `setViewport` at the store level. </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 - Reviewed through the security lens. The new editor canvas renders mock/own-user data behind `requireAuth` (router.tsx:92). No `dangerouslySetInnerHTML`/`innerHTML` anywhere in the diff; SVG `<text>` content (`object.name`) and attribute values (`object.color`, `kind`-derived fills, transform strings) flow through React's escaping and are constructed only from numeric/typed fields — no string interpolation into executable context. No SQL/command/template injection surface, no SSRF, no deserialization, no secrets. Scale/viewport math operates on `number`s with clamping (`clampScale`, `MIN_SCALE`/`MAX_SCALE`), so no unbounded-state concern in my lane. - Verified by reading `web/src/editor/ObjectShape.tsx`, `types.ts`, `GardenEditorPage.tsx`, and grepping the route guard (`web/src/router.tsx:89-94`) plus `dangerouslySetInnerHTML`/`innerHTML` (none). `EditorObject.color` is the only externally-supplied-presentational field that will later come from the server; React renders it as a plain attribute value, which does not execute (no `javascript:`-URL sink for shape `fill`). Not a finding for this PR. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I'll examine the geometry and viewport logic for correctness issues. Now let me verify the key math. The `ObjectShape` circle uses `r={halfW}` (width/2), ignoring height. And `worldToLocal`/`localToWorld` inverses: I have enough context. Let me now analyze the correctness carefully through my lens. **Rotation direction verification:** `localToWorld` rotates `{x:10,y:0}` by 90°: cos90=0, sin90=1 → `x = 500 + 10*0 - 0*1 = 500`, `y = 300 + 10*1 + 0*0 = 310`. That's 10 below center. On a y-down screen, rotating the +x axis 90° clockwise should land pointing down (+y). ✓ Correct. **`zoomViewportAt`:** Keeps world point under cursor stationary. world = screenToWorld(point, vp); then tx = point.x - world.x*scale. New screen = world*scale + tx = world*scale + point.x - world.x*scale = point.x. ✓ Correct even after clamp (since world is computed from the *old* vp, the new tx compensates for the clamped scale). **`zoomToFitRect` centering:** center maps to viewport center: screen = center*scale + tx; tx = viewport.w/2 - center*scale → screen = viewport.w/2. ✓ **Circle radius:** Uses `r={halfW}` = widthCm/2. For a circle, "width" represents diameter; radius = width/2. ✓ Reasonable convention. **Grid threshold:** `showGrid = GRID_CM * viewport.scale >= GRID_MIN_CELL_PX` = 100*scale >= 6 → scale >= 0.06. Min scale is 0.05, so at extreme min zoom grid hides. ✓ Reasonable. Now let me look for issues: **Issue 1 — `worldToLocal`/`localToWorld` inverse:** Verified by round-trip test and manually: inverse of rotation by angle θ is rotation by -θ. localToWorld uses `[cosθ, -sinθ; sinθ, cosθ]`. worldToLocal: `[cosθ, sinθ; -sinθ, cosθ]` = rotation by -θ. ✓ Correct. **Issue 2 — Wheel zoom direction:** `Math.exp(-dy * WHEEL_SENSITIVITY)`. Standard wheel: dy>0 = scroll down = zoom out. exp(-positive) < 1 → scale decreases. ✓ Correct. **Issue 3 — fitToRect clamping during animation:** The animation lerps scale linearly from `from.scale` to `to.scale` (clamped target). But the intermediate scale values are NOT clamped — if `from.scale` exceeds bounds (shouldn't, since input is always clamped) it's fine. Actually a subtle issue: the final value at t=1 is `to.scale` which is clamped. But `lerp(from.scale, to.scale, 1) === to.scale`. ✓ Fine. **Issue 4 — `fitToRect` dependency & re-fit:** `fittedRef` prevents auto-fit only once, but if garden changes (different widthCm), no re-fit. Out of scope for mock. Not a correctness bug in this PR. **Issue 5 — Drag handler `pinching` guard:** On `onDrag`, checks `pinching` and cancels. ✓ **Issue 6 — `useGesture` `from` for pinch:** `from: () => [scale, 0]` returns array. use-gesture pinch `from` expects `[da, db]` where da is scale. ✓ **Issue 7 — `ObjectShape` circle with non-equal width/height:** Circle uses `r=halfW` ignoring height. If a circle object has width≠height it renders as a circle based on width only — visually inconsistent with bbox. Minor; mock data uses equal (120×120). Not a real bug for current data. **Issue 8 — `text` rotation:** Text is inside the rotated `<g>`, so the label rotates with the object. For a 30° bed "Angled", the text is rotated 30°. That's intentional (label rotates with object). Not a bug. **Issue 9 — `fontCm` units:** fontSize in cm units (world space), then scaled by viewport. `Math.min(width, height)*0.28` — for a 120cm circle, fontCm = 33.6cm. At scale 0.5 px/cm → 16.8px. Reasonable. ✓ **Issue 10 — `screenToWorld` at the cursor for wheel:** uses `toLocal(event.clientX, event.clientY)` which subtracts svg bounding rect → svg-local pixels. `worldToScreen`/`screenToWorld` use svg-local pixel space (since the `<g transform>` is within svg coords). ✓ Consistent. Let me double-check the `zoomViewportAt` clamping edge: when `nextScale` is clamped, the stationary-point property still holds because `world` is from the OLD vp, and the new `tx/ty` are derived from the clamped `scale`. So the world point under the cursor stays put even when clamping kicks in. ✓ Correct (and the test confirms scale clamps to 20 at extreme). Now let me check one potential real issue: the wheel handler uses `event.clientX/Y` but `event` here — is it a WheelEvent? use-gesture's `onWheel` event is a WheelEvent with clientX/Y. ✓ Let me check the `delta: [, dy]` destructure for wheel — use-gesture wheel delta is `[dx, dy]` (horizontal, vertical). ✓ Let me verify the units test expectations re-derivation: `cmFromFtIn(4)`: (4*12+0)*2.54 = 121.92 → round 122. ✓ `cmFromFtIn(4,6)`: (48+6)*2.54 = 137.16 → 137. ✓ `formatCm(122,'imperial')`: 122/2.54 = 48.0315 in; feet=4, inches=round(0.0315)=0 → "4′ 0″". ✓ But test expects "4′ 0″". ✓ `formatCm(137,'imperial')`: 137/2.54 = 53.937 in; feet=4, inches=round(53.937-48)=round(5.937)=6 → "4′ 6″". ✓ Now, one subtle correctness issue in `formatCm` imperial: `inches = Math.round(totalInches - feet * INCHES_PER_FOOT)`. If totalInches = 47.9 (just under 4ft), feet=3, inches=round(11.9)=12 → carries to 4′ 0″. ✓ Handled. Let me look at the `displayFromCm` imperial rounding — rounds to 0.1 ft. ✓ Everything in my lens checks out. Let me check the `wheel` event's `delta` sign convention once more — actually different browsers/devices vary, but `Math.exp(-dy*SENS)` with dy>0 (scroll down/away) zooms out, which is the standard trackpad expectation. Not a bug. Let me verify one final potential issue — `useGesture` config `eventOptions: { passive: false }` is applied globally; with `target: svgRef` and React also having `onPointerDown` on the svg. The gesture lib binds its own listeners via `target`. React's `onPointerDown` is separate (delegated). Both fire. Object pointerdown calls `stopPropagation()` — but does that stop use-gesture's drag from starting? use-gesture attaches listeners to the target (svg) in capture/bubble phase via native addEventListener. React's synthetic `stopPropagation` stops the React synthetic event propagation but the native event continues to bubble to document where use-gesture... actually use-gesture binds to the target element directly. Hmm. This could mean dragging an object also pans the canvas. But this is interaction behavior (#11's domain), and the diff notes object dragging stops propagation. Let me not over-reach — this is borderline outside pure correctness of the new math, and it's explicitly deferred to #11. Based on my correctness lens (math, transforms, units, formulas), everything verifies correctly. VERDICT: No material issues found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> VERDICT: Minor issues - **`web/src/editor/useViewport.ts:74`** — `fitToRect(rect: Rect, viewport: Size)` names its second parameter `viewport`, but its type is `Size` (`{w, h}`). The codebase establishes `Viewport` (`{tx, ty, scale}`) as a core type in `geometry.ts` (line 25) and `useViewport.ts` works with that `Viewport` concept throughout (e.g. lines 47, 53, 76). Using `viewport` to mean "canvas pixel size" is a real readability trap at the call sites `fitToRect(gardenRect, size)` in `GardenCanvas.tsx:48,110`. (Note: `zoomToFitRect` in `geometry.ts:99` reuses the same convention, so this is a pre-existing smell the new code inherits rather than introduces.) Rename to `viewSize` or `canvasSize`. Verified. - **`web/src/editor/GardenCanvas.tsx:13`** — doc comment says "a fading 1 m grid", but the grid just toggles on/off via `showGrid` (`GRID_CM * viewport.scale >= GRID_MIN_CELL_PX`, line 52) with constant `strokeOpacity={0.18}` (line 74). There is no fade. The comment will mislead maintainers. Either implement a fade (opacity as a function of cell px) or drop "fading". Verified. - **`web/src/editor/ObjectShape.tsx:40,70`** — magic numbers `0.28`, `8`, `40` (label sizing, line 40) and `0.06` (corner radius, line 70) only have a one-line comment on the former. Fine to keep but a named constant or a brief `// 0.06 → ~6% of the shorter side` would help future tuners. Minor. - **`web/src/editor/useViewport.ts:20`** — the hook is named `useViewport` but returns only `{ fitToRect }` (line 92); the viewport state itself lives in the zustand store (`useEditorStore`). A reader expecting `useViewport()` to surface viewport state will be surprised. Consider `useViewportGestures` or have it also return the viewport. Minor naming smell. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I'll review this through the performance lens, verifying with the repo tools. I have enough to evaluate. Let me check the GardenEditorPage mock objects to confirm scale of data. # Verdict: No material issues found Performance lens review of this canvas foundation slice. Mock data only, six objects. Even extrapolated to plausible garden sizes the hot paths are cheap. Verified by reading the actual files: - `GardenCanvas.tsx:53` — `ordered` is `useMemo`'d on `[objects]` and does a single `sort()`. O(n log n) once per objects change, not per render. Fine. - `useViewport.ts:39-71` — wheel/drag/pinch handlers call `setViewport` with functional updates; no per-event allocations of concern beyond the trivial viewport object (three numbers). `toLocal` calls `getBoundingClientRect()` per wheel/pinch event — this forces a layout read but only on actual user input events (not a hot inner loop), and it's the standard, acceptable pattern. Acceptable. - `useViewport.ts:73-88` — fit animation runs rAF for 350ms, lerps 3 numbers, calls `setViewport` per frame. Bounded duration, cancels on unmount and on any gesture start. Clean. - `store.ts:24-25` — `setViewport` spreads `{...vp}` in the drag path creating one small object per drag delta. Negligible; standard Zustand usage. - Grid rendering uses a single SVG `<pattern>` fill rather than per-cell lines — good, scales with viewport size not cell count. `showGrid` gates it (`GardenCanvas.tsx:52`) so it disappears when zoomed far out. - `ObjectShape` is memo-free but only re-renders when its props change; the `.map` in `GardenCanvas.tsx:97` keys on `o.id`. With small N this is fine; if N ever grows large, wrapping `ObjectShape` in `React.memo` would be worth it, but that's premature now and out of scope for this lens given current data. No N+1, no unbounded growth, no blocking calls on hot paths, no quadratic behavior. Nothing material in the performance lane. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> VERDICT: No material issues found I reviewed the diff through the error-handling / edge-case lens, reading `geometry.ts`, `useViewport.ts`, and `GardenCanvas.tsx` in full to verify. The unhappy paths are handled correctly: - **Division by zero in `screenToWorld`** (`geometry.ts:35-37`): impossible in practice — `scale` is seeded at `1` and every mutation routes through `clampScale` with `MIN_SCALE = 0.05`. Verified the store (`store.ts:29`) and both callers (`useViewport.ts:53, 58`). - **`zoomToFitRect` with zero/negative viewport or rect** (`geometry.ts:97-117`): `availW/availH` clamped to `≥1`, `rectW/rectH` clamped to `≥1e-6`, and `scale` clamped to `[minScale, maxScale]`. No NaN/Infinity escapes. Callers also guard `size.w > 0 && size.h > 0` (`GardenCanvas.tsx:46, 110`). - **Wheel overflow** (`useViewport.ts:53`): `Math.exp(-dy * WHEEL_SENSITIVITY)` can tend to 0 or Infinity for extreme `dy`, but the resulting scale is run through `clampScale` → safe. - **Fit tween lifecycle** (`useViewport.ts:73-90`): `cancelAnim` runs at the start of each `fitToRect` (so repeat clicks don't stack rAFs) and on unmount via `useEffect(() => cancelAnim, [cancelAnim])` — no orphaned rAF writing to Zustand after unmount. `t` is clamped to `[0,1]` so a backgrounded tab catching up doesn't overshoot. - **ResizeObserver** (`GardenCanvas.tsx:34-42`): null-element guard, proper `disconnect()` cleanup; the first-fit effect gates on `size.w > 0 && size.h > 0` so a hidden/zero-size container won't fit to a degenerate rect. `fittedRef` prevents re-fitting on unrelated size changes. - **Pinch `from` seeding** (`useViewport.ts:67`): reads `useEditorStore.getState().viewport.scale` — always defined; no nil dereference even on first pinch. - **`toLocal`** (`useViewport.ts:31-37`): falls back to raw client coords if `getBoundingClientRect()` is unavailable — defensive, no crash. The only thing I'd flag as *outside* strict error-handling (and therefore not a finding): `ObjectShape` doesn't validate `widthCm/heightCm > 0`, so a degenerate object would render with `r=0` / negative dimensions — but that's a data-validation concern, not an unhandled error in the introduced paths, and the mock data is well-formed. No swallowed errors, no missing cleanup, no panic-prone inputs in this change. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found - I reviewed all new and changed files through the security lens. The only potential injection surface I identified is `object.color` interpolated into SVG `fill` attributes (`ObjectShape.tsx:55/71`), which is typed as `string | null | undefined` with no runtime validation. SVG `fill` can accept `url(...)` references, so an arbitrary string would be a paint-value injection vector (SSRF-like requests, tracking pixels) once #11 makes this field editable. However, **in this PR the field is never populated**—mock objects omit `color`, and the field editor that would accept user input is explicitly out of scope for #9. The vulnerability surface is latent, not live. - No authn/authz gaps: `GardenEditorPage` remains behind the existing `requireAuth` guard; no new unprotected routes are introduced. - No unsafe deserialization, command/SQL injection, path traversal, SSRF, secret leakage, or missing input validation on active code paths. - Dependencies (`zustand`, `@use-gesture/react`, `vitest`) are standard, well-maintained packages with no known security red flags in the versions added. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict: No material issues found** I scrutinized the geometry transforms, zoom math, unit conversions, and gesture handling. Everything checks out. - **`localToWorld` / `worldToLocal`**: The rotation matrix is the standard 2D rotation. In screen coordinates (y-down), this produces clockwise rotation for positive angles, matching the documented behavior. Verified the 90° test case round-trips correctly. - **`zoomViewportAt`**: The algebra `tx' = screenPoint.x - world.x * scale'` correctly preserves the world point under the cursor. Verified against the test case: `screenToWorld(cursor, next)` round-trips to the original world point. - **`zoomToFitRect`**: Centers the rect by mapping `rectCenter` to `viewportCenter` with `tx = viewport.w/2 - centerX*scale`. Verified with the test assertions. - **Unit conversions**: `cmFromFtIn`, `cmFromMeters`, `displayFromCm`, and `formatCm` all use correct constants (`2.54 cm/in`, `12 in/ft`, `100 cm/m`) and appropriate rounding. Round-trip drift is within expected quantization bounds. - **Wheel/pinch gesture math**: The exponential zoom `scale * exp(-dy * sensitivity)` is standard and directionally correct (scroll-up zooms in). The `from` seed for pinch ensures absolute scale values. No logic bugs, incorrect formulas, or semantic domain errors found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - `web/src/editor/GardenCanvas.tsx:69` — The SVG `<pattern id="garden-grid">` uses a hard-coded `id`. If `GardenCanvas` is ever rendered more than once per page (or remounted), the pattern ID will collide in the DOM. Use `React.useId()` and reference it via `url(#${gridId})` instead. - `web/src/editor/GardenCanvas.tsx:21,45-50` — `fittedRef` gates the auto-fit effect so it only fires once per component lifetime. If the `garden` prop changes (which #11 will support), the canvas silently stays framed for the old garden. Track the last fitted garden ID and reset the flag when `garden.id` changes, or drive fit state from a stable key. - `web/src/editor/useViewport.ts:74` — The `fitToRect` callback names its second parameter `viewport` but its type is `Size` (pixel dimensions). This collides semantically with the `Viewport` transform type used throughout the codebase. Rename it to `viewSize`, `containerPx`, or similar to disambiguate. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> Minor issues found * **Forced synchronous layout in gesture hot path** — `web/src/editor/useViewport.ts:33` The `toLocal` helper calls `svgRef.current?.getBoundingClientRect()` inside every `onWheel` and `onPinch` event. During a pinch gesture this fires on every touch-move frame, forcing the browser to synchronously recalculate layout (reflow) while handling input. On lower-end devices this produces jank. **Fix:** cache the SVG bounding offset (e.g. in a ref updated on mount/resize via `ResizeObserver` or the existing observer in `GardenCanvas`) and read the cached value in `toLocal` instead of calling `getBoundingClientRect()` in the handler. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`web/src/editor/store.ts:24`** — `setViewport` accepts any `Viewport` without clamping `scale` to `[MIN_SCALE, MAX_SCALE]`. A direct call with `scale: 0` (or `NaN`/negative) corrupts the canvas permanently because downstream `screenToWorld` divides by `vp.scale`, yielding `Infinity`/`NaN`. The store should enforce the invariant it exports, either by clamping inside `setViewport` or by rejecting non-finite values. - **`web/src/editor/useViewport.ts:33-34`** — `toLocal` silently falls back to raw `clientX`/`clientY` when `svgRef.current` is null (unmount timing, ref race, or conditional rendering). This makes wheel/pinch zoom anchor to the browser viewport origin instead of the SVG, producing a jarring jump. It should return a sentinel or throw rather than lie about local coordinates. - **`web/src/lib/geometry.ts:104-108`** — `zoomToFitRect` silently misbehaves on negative `rect` dimensions. `Math.max(rect.w, 1e-6)` masks the width for scale math, but `centerX = rect.x + rect.w / 2` (line 110) still uses the negative original, centering the viewport on the wrong point. Should validate `rect.w > 0 && rect.h > 0` (or take absolute values) and error/warn instead of producing a misleading viewport. - **`web/src/editor/ObjectShape.tsx:37-40`** — No guard against zero or negative `widthCm`/`heightCm`. A zero-width circle gets `r={0}` (invisible); a negative-width rect flips its `x` origin and yields a negative `rx`, which SVG treats as an error. The component should clamp dimensions to `> 0` or skip rendering invalid geometry. - **`web/src/editor/useViewport.ts:53`** — `onWheel` does not guard against `NaN` in `dy`. A synthetic or broken wheel event with `dy = NaN` propagates `NaN` through `Math.exp` → `clampScale` → `zoomViewportAt`, permanently poisoning the viewport state. A `Number.isFinite(dy)` check (and on the resulting scale) would prevent this. - **`web/src/editor/GardenCanvas.tsx:46-49`** — The auto-fit `useEffect` only checks canvas `size` positivity, not `gardenRect` validity. If `garden.widthCm` is `NaN` (malformed mock or future server data), `zoomToFitRect` returns a `NaN` viewport and the canvas becomes unrecoverably blank. Should guard with `Number.isFinite(gardenRect.w) && gardenRect.w > 0` (same for `h`). </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-19 00:16:55 +00:00
Fixes from the PR #29 adversarial review (considered; not graded).

Performance / correctness
- ObjectShape is memo'd, so a pan/zoom (which only mutates the world <g>
  transform) no longer re-renders every object.
- 'circle' objects render as an <ellipse> (rx/ry), honoring both dims — a
  circle when width == height — instead of ignoring heightCm.
- The 1 m grid now actually fades in (opacity ramps as cells grow past the
  visibility threshold), matching its description.
- Unique SVG pattern id (useId) so multiple canvases can't collide.
- The canvas re-fits when the garden id changes (not just once), so #11
  switching gardens reframes.

Robustness
- geometry.zoomToFitRect takes rect size by magnitude; wheel/pinch ignore
  non-finite deltas; ObjectShape clamps dimensions to >= 0 (no invalid SVG).

Maintainability
- Renamed the Size params from `viewport` to `canvasSize` (4 models); moved
  easeInOutCubic/lerp into geometry.ts; renamed toLocal → clientToCanvas;
  named constants for the label/corner factors; DEFAULT_FILL const;
  EditorGarden.unitPref imports UnitPref; the Fit control uses the shared
  Button. focusedObjectId/plantable are intentionally reserved for #11/#15.

tsc + 17 vitest tests green; re-verified in a browser (ellipses render,
unique pattern id, wheel zoom + Fit).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit 2119f1a376 into main 2026-07-19 00:18:28 +00:00
steve deleted branch phase-3-canvas 2026-07-19 00:18:28 +00:00
Sign in to join this conversation.