Plops editor: focus mode, place/move/resize, semantic zoom (#15) #34

Merged
steve merged 2 commits from phase-5-plops-editor into main 2026-07-19 03:22:51 +00:00
10 changed files with 115 additions and 76 deletions
Showing only changes of commit 0f67623da7 - Show all commits
+6 -8
View File
@@ -8,6 +8,7 @@ import { PlopLayer } from './PlopLayer'
import { PlopOverlay } from './PlopOverlay' import { PlopOverlay } from './PlopOverlay'
import { SelectionOverlay } from './SelectionOverlay' import { SelectionOverlay } from './SelectionOverlay'
import { kindDef } from './kinds' import { kindDef } from './kinds'
import { DIMMED_OPACITY, objectTransform } from './shared'
import { useEditorStore } from './store' import { useEditorStore } from './store'
import { useViewport } from './useViewport' import { useViewport } from './useViewport'
import type { EditorGarden, EditorObject } from './types' import type { EditorGarden, EditorObject } from './types'
@@ -15,7 +16,6 @@ import type { EditorGarden, EditorObject } from './types'
const GRID_CM = 100 // 1 m grid const GRID_CM = 100 // 1 m grid
const GRID_MIN_CELL_PX = 6 const GRID_MIN_CELL_PX = 6
const GRID_MAX_OPACITY = 0.18 const GRID_MAX_OPACITY = 0.18
Outdated
Review

🟡 Focus dimming opacity inconsistent across components

maintainability · flagged by 2 models

🪰 Gadfly · advisory

🟡 **Focus dimming opacity inconsistent across components** _maintainability · flagged by 2 models_ <sub>🪰 Gadfly · advisory</sub>
const DIMMED_OPACITY = 0.35
/** The world-space bounds rect of an object (center + size → top-left rect). */ /** The world-space bounds rect of an object (center + size → top-left rect). */
function objectRect(o: EditorObject): Rect { function objectRect(o: EditorObject): Rect {
@@ -33,12 +33,12 @@ export function GardenCanvas({
garden, garden,
objects, objects,
plantings, plantings,
plants, plantsById,
}: { }: {
garden: EditorGarden garden: EditorGarden
objects: EditorObject[] objects: EditorObject[]
plantings: EditorPlanting[] plantings: EditorPlanting[]
plants: Plant[] plantsById: Map<number, Plant>
}) { }) {
const svgRef = useRef<SVGSVGElement>(null) const svgRef = useRef<SVGSVGElement>(null)
const containerRef = useRef<HTMLDivElement>(null) const containerRef = useRef<HTMLDivElement>(null)
@@ -65,8 +65,6 @@ export function GardenCanvas({
[garden.widthCm, garden.heightCm], [garden.widthCm, garden.heightCm],
) )
const plantsById = useMemo(() => new Map(plants.map((p) => [p.id, p])), [plants])
useEffect(() => { useEffect(() => {
Outdated
Review

🟡 Duplicate plantsById construction in GardenCanvas

maintainability · flagged by 2 models

  • GardenCanvas rebuilds plantsById that its parent already computed (web/src/editor/GardenCanvas.tsx:68). GardenEditorPage constructs the same Map and passes the raw plants array down only so the canvas can rebuild it. Passing plantsById directly removes duplicate code and a redundant useMemo.

🪰 Gadfly · advisory

🟡 **Duplicate plantsById construction in GardenCanvas** _maintainability · flagged by 2 models_ - **`GardenCanvas` rebuilds `plantsById` that its parent already computed** (`web/src/editor/GardenCanvas.tsx:68`). `GardenEditorPage` constructs the same `Map` and passes the raw `plants` array down only so the canvas can rebuild it. Passing `plantsById` directly removes duplicate code and a redundant `useMemo`. <sub>🪰 Gadfly · advisory</sub>
const el = containerRef.current const el = containerRef.current
if (!el) return if (!el) return
@@ -136,7 +134,7 @@ export function GardenCanvas({
// armed for repeat-placement until Escape / Done). // armed for repeat-placement until Escape / Done).
function onPlace(e: ReactPointerEvent) { function onPlace(e: ReactPointerEvent) {
e.stopPropagation() e.stopPropagation()
if (!focusedObject || !armedPlant) return if (!focusedObject || !armedPlant || !focusedObject.plantable) return
const rect = svgRef.current?.getBoundingClientRect() const rect = svgRef.current?.getBoundingClientRect()
if (!rect) return if (!rect) return
Outdated
Review

🟡 onPlace allows planting on non-plantable objects when deep-linked

correctness · flagged by 1 model

  • web/src/editor/GardenCanvas.tsx:139onPlace guards with if (!focusedObject || !armedPlant) return but never checks focusedObject.plantable. A deep-link (?focus=<id>) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt. Fix: add || !focusedObject.plantable to the guard.

🪰 Gadfly · advisory

🟡 **onPlace allows planting on non-plantable objects when deep-linked** _correctness · flagged by 1 model_ - **`web/src/editor/GardenCanvas.tsx:139`** — `onPlace` guards with `if (!focusedObject || !armedPlant) return` but never checks `focusedObject.plantable`. A deep-link (`?focus=<id>`) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt. **Fix:** add `|| !focusedObject.plantable` to the guard. <sub>🪰 Gadfly · advisory</sub>
const world = screenToWorld({ x: e.clientX - rect.left, y: e.clientY - rect.top }, viewport) const world = screenToWorld({ x: e.clientX - rect.left, y: e.clientY - rect.top }, viewport)
1
@@ -197,8 +195,8 @@ export function GardenCanvas({
{/* Placement capture: a transparent sheet over the focused object while a {/* Placement capture: a transparent sheet over the focused object while a
plant is armed, so taps drop plops instead of selecting the object. */} plant is armed, so taps drop plops instead of selecting the object. */}
{focusedObject && armedPlant && ( {focusedObject && armedPlant && focusedObject.plantable && (
<g transform={`translate(${focusedObject.xCm} ${focusedObject.yCm}) rotate(${focusedObject.rotationDeg})`}> <g transform={objectTransform(focusedObject)}>
<rect <rect
x={-halfFW} x={-halfFW}
y={-halfFH} y={-halfFH}
+2 -5
View File
@@ -1,4 +1,5 @@
import { memo, type PointerEvent } from 'react' import { memo, type PointerEvent } from 'react'
import { objectTransform } from './shared'
import type { EditorObject } from './types' import type { EditorObject } from './types'
const DEFAULT_FILL = '#8a8a8a' const DEFAULT_FILL = '#8a8a8a'
@@ -60,11 +61,7 @@ export const ObjectShape = memo(function ObjectShape({
const strokeWidth = selected ? 2 : 1 const strokeWidth = selected ? 2 : 1
return ( return (
<g <g transform={objectTransform(object)} onPointerDown={handleDown} style={{ cursor: 'pointer' }}>
transform={`translate(${object.xCm} ${object.yCm}) rotate(${object.rotationDeg})`}
onPointerDown={handleDown}
style={{ cursor: 'pointer' }}
>
{object.shape === 'circle' ? ( {object.shape === 'circle' ? (
<ellipse <ellipse
cx={0} cx={0}
+41 -23
View File
@@ -1,19 +1,22 @@
import { useEffect, useRef, useState } from 'react' import { useEffect, useRef, useState } from 'react'
import { Button } from '@/components/ui/Button' import { Button } from '@/components/ui/Button'
import { TextField } from '@/components/ui/TextField' import { TextField } from '@/components/ui/TextField'
import { PlantIcon } from '@/components/plants/PlantIcon'
import { useRemovePlanting, useUpdatePlanting } from '@/lib/objects' import { useRemovePlanting, useUpdatePlanting } from '@/lib/objects'
import type { Plant } from '@/lib/plants' import type { Plant } from '@/lib/plants'
import { computeDerivedCount, type EditorPlanting } from '@/lib/plantings' import { computeDerivedCount, type EditorPlanting } from '@/lib/plantings'
import { cmFromSpacing, spacingFromCm, spacingUnitLabel, type UnitPref } from '@/lib/units' import { cmFromSpacing, spacingFromCm, spacingUnitLabel, type UnitPref } from '@/lib/units'
import { PlantIcon } from '@/components/plants/PlantIcon' import { MIN_RADIUS_CM } from './shared'
import { useEditorStore } from './store'
Outdated
Review

🟡 MIN_RADIUS_CM duplicated verbatim between PlopInspector.tsx and PlopOverlay.tsx, both new in this PR

maintainability · flagged by 2 models

  • web/src/editor/PlopOverlay.tsx:11 / web/src/editor/PlopMarker.tsx:12const SELECT_COLOR = '#2f7a3e' is now defined identically in three places: the pre-existing SelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. editor/colors.ts) or exporting it from SelectionOverlay.tsx. - **web/src/editor/PlopInspector.tsx:10 and `w…

🪰 Gadfly · advisory

🟡 **MIN_RADIUS_CM duplicated verbatim between PlopInspector.tsx and PlopOverlay.tsx, both new in this PR** _maintainability · flagged by 2 models_ - **`web/src/editor/PlopOverlay.tsx:11` / `web/src/editor/PlopMarker.tsx:12`** — `const SELECT_COLOR = '#2f7a3e'` is now defined identically in three places: the pre-existing `SelectionOverlay.tsx:11`, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. `editor/colors.ts`) or exporting it from `SelectionOverlay.tsx`. - **`web/src/editor/PlopInspector.tsx:10` and `w… <sub>🪰 Gadfly · advisory</sub>
const MIN_RADIUS_CM = 1
/** /**
* Property panel for the selected plop. Radius/label/date/count commit a PATCH on * Property panel for the selected plop. Radius/label/date/count commit a PATCH on
* blur (carrying the version); the count field shows the live derived value as a * blur (carrying the version); the count field shows the live derived value as a
* placeholder and takes an override when typed. "Remove" soft-removes (keeps the * placeholder and takes an override when typed. "Remove" soft-removes (keeps the
* row with removed_at); "Change plant" opens the picker via onChangePlant. * row with removed_at); "Change plant" opens the picker via onChangePlant. While
* the plop is dragged/resized on the canvas, it reads livePlanting for live
* feedback (subscribed here, not at the page level, so a drag only re-renders
* this panel).
*/ */
export function PlopInspector({ export function PlopInspector({
plop, plop,
@@ -32,44 +35,59 @@ export function PlopInspector({
}) { }) {
const update = useUpdatePlanting(gardenId) const update = useUpdatePlanting(gardenId)
const remove = useRemovePlanting(gardenId) const remove = useRemovePlanting(gardenId)
Review

🟡 Four useState initializers duplicated verbatim in the resync effect; extract a sync helper

maintainability · flagged by 1 model

  • web/src/editor/PlopInspector.tsx:37-40 + 46-49 — The four useState initializers are duplicated verbatim in the resync useEffect. Extract a syncFromPlop(plop, unit) helper (or a small useSyncedFields hook) to keep the two sites from drifting.

🪰 Gadfly · advisory

🟡 **Four useState initializers duplicated verbatim in the resync effect; extract a sync helper** _maintainability · flagged by 1 model_ - **`web/src/editor/PlopInspector.tsx:37-40`** + **`46-49`** — The four `useState` initializers are duplicated verbatim in the resync `useEffect`. Extract a `syncFromPlop(plop, unit)` helper (or a small `useSyncedFields` hook) to keep the two sites from drifting. <sub>🪰 Gadfly · advisory</sub>
const livePlanting = useEditorStore((s) => s.livePlanting)
const rootRef = useRef<HTMLDivElement>(null) const rootRef = useRef<HTMLDivElement>(null)
const [radius, setRadius] = useState(String(spacingFromCm(plop.radiusCm, unit))) // The plop with any in-flight drag/resize geometry applied.
const [count, setCount] = useState(plop.count != null ? String(plop.count) : '') const p = livePlanting && livePlanting.id === plop.id ? livePlanting : plop
const [label, setLabel] = useState(plop.label ?? '')
const [planted, setPlanted] = useState(plop.plantedAt ?? '') const [radius, setRadius] = useState(String(spacingFromCm(p.radiusCm, unit)))
const [count, setCount] = useState(p.count != null ? String(p.count) : '')
const [label, setLabel] = useState(p.label ?? '')
const [planted, setPlanted] = useState(p.plantedAt ?? '')
// Re-sync when the plop changes underneath us (a drag/resize or a server row), // Re-sync when the plop changes underneath us (a drag/resize or a server row),
// unless the user is editing a field here. // unless the user is editing a field here.
useEffect(() => { useEffect(() => {
if (rootRef.current?.contains(document.activeElement)) return if (rootRef.current?.contains(document.activeElement)) return
setRadius(String(spacingFromCm(plop.radiusCm, unit))) setRadius(String(spacingFromCm(p.radiusCm, unit)))
setCount(plop.count != null ? String(plop.count) : '') setCount(p.count != null ? String(p.count) : '')
setLabel(plop.label ?? '') setLabel(p.label ?? '')
setPlanted(plop.plantedAt ?? '') setPlanted(p.plantedAt ?? '')
}, [plop.version, plop.radiusCm, plop.count, plop.label, plop.plantedAt, unit]) }, [p.version, p.radiusCm, p.count, p.label, p.plantedAt, unit])
const patch = (fields: Omit<Parameters<typeof update.mutate>[0], 'id' | 'version'>) => const patch = (fields: Omit<Parameters<typeof update.mutate>[0], 'id' | 'version'>) =>
Review

🟠 commitRadius uses parseFloat which accepts partial parses like '1.2.3' without user feedback

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟠 **commitRadius uses parseFloat which accepts partial parses like '1.2.3' without user feedback** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
update.mutate({ id: plop.id, version: plop.version, ...fields }) update.mutate({ id: plop.id, version: plop.version, ...fields })
const u = spacingUnitLabel(unit) const u = spacingUnitLabel(unit)
const derived = plant ? computeDerivedCount(plop.radiusCm, plant.spacingCm) : plop.derivedCount const derived = plant ? computeDerivedCount(p.radiusCm, plant.spacingCm) : p.derivedCount
function commitRadius() { function commitRadius() {
Review

🟡 commitCount silently drops invalid input with no reset or feedback, unlike commitRadius which clamps

error-handling · flagged by 1 model

  • web/src/editor/Inspector.tsx:227 + web/src/pages/GardenEditorPage.tsx:78-99,118,140 + web/src/editor/GardenCanvas.tsx:81-89,178 — Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed: ObjectShape.tsx has no focus-mode gating on onPointerDown — the focused object itself renders at full opacity (GardenCanvas.tsx:178, focusedObjectId !== o.id dimming only applies to other objects) and is fully selectable via the plain select action…

🪰 Gadfly · advisory

🟡 **commitCount silently drops invalid input with no reset or feedback, unlike commitRadius which clamps** _error-handling · flagged by 1 model_ - **`web/src/editor/Inspector.tsx:227` + `web/src/pages/GardenEditorPage.tsx:78-99,118,140` + `web/src/editor/GardenCanvas.tsx:81-89,178`** — Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed: `ObjectShape.tsx` has no focus-mode gating on `onPointerDown` — the focused object itself renders at full opacity (`GardenCanvas.tsx:178`, `focusedObjectId !== o.id` dimming only applies to *other* objects) and is fully selectable via the plain `select` action… <sub>🪰 Gadfly · advisory</sub>
const v = parseFloat(radius) const v = Number(radius)
if (!Number.isFinite(v)) return if (radius.trim() === '' || !Number.isFinite(v)) {
setRadius(String(spacingFromCm(p.radiusCm, unit))) // reset stale/invalid text
return
}
Outdated
Review

🟡 commitCount silently rejects invalid/float input without user feedback

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟡 **commitCount silently rejects invalid/float input without user feedback** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
const cm = Math.max(MIN_RADIUS_CM, cmFromSpacing(v, unit)) const cm = Math.max(MIN_RADIUS_CM, cmFromSpacing(v, unit))
if (cm !== plop.radiusCm) patch({ radiusCm: cm }) if (cm !== p.radiusCm) patch({ radiusCm: cm })
} }
function commitCount() { function commitCount() {
if (count.trim() === '') { if (count.trim() === '') {
if (plop.count != null) patch({ count: null }) // restore derived if (p.count != null) patch({ count: null }) // restore derived
return return
} }
const n = Number(count) const n = Number(count)
if (!Number.isInteger(n) || n < 1) return if (!Number.isInteger(n) || n < 1) {
if (n !== plop.count) patch({ count: n }) setCount(p.count != null ? String(p.count) : '') // reject invalid, restore
return
}
if (n !== p.count) patch({ count: n })
}
function commitPlanted() {
const next = planted === '' ? null : planted
if (next !== (p.plantedAt ?? null)) patch({ plantedAt: next })
} }
return ( return (
@@ -121,7 +139,7 @@ export function PlopInspector({
value={count} value={count}
onChange={(e) => setCount(e.target.value)} onChange={(e) => setCount(e.target.value)}
onBlur={commitCount} onBlur={commitCount}
hint={plop.count != null ? 'Override — clear for auto' : 'Auto from area ÷ spacing²'} hint={p.count != null ? 'Override — clear for auto' : 'Auto from area ÷ spacing²'}
Outdated
Review

🔴 Cannot clear plantedAt date: blur guard prevents null patch and re-sync snaps value back

correctness, error-handling · flagged by 4 models

  • web/src/editor/PlopInspector.tsx:142Cannot clear the "Planted" date once set. The onBlur handler guards with planted !== '', so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should send plantedAt: planted || null and remove the empty-string guard.

🪰 Gadfly · advisory

🔴 **Cannot clear plantedAt date: blur guard prevents null patch and re-sync snaps value back** _correctness, error-handling · flagged by 4 models_ * **`web/src/editor/PlopInspector.tsx:142`** — **Cannot clear the "Planted" date once set.** The `onBlur` handler guards with `planted !== ''`, so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should send `plantedAt: planted || null` and remove the empty-string guard. <sub>🪰 Gadfly · advisory</sub>
/> />
</div> </div>
@@ -130,7 +148,7 @@ export function PlopInspector({
name="label" name="label"
value={label} value={label}
onChange={(e) => setLabel(e.target.value)} onChange={(e) => setLabel(e.target.value)}
onBlur={() => label !== (plop.label ?? '') && patch({ label: label.trim() || null })} onBlur={() => label !== (p.label ?? '') && patch({ label: label.trim() || null })}
/> />
<TextField <TextField
@@ -139,7 +157,7 @@ export function PlopInspector({
type="date" type="date"
value={planted} value={planted}
onChange={(e) => setPlanted(e.target.value)} onChange={(e) => setPlanted(e.target.value)}
onBlur={() => planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted })} onBlur={commitPlanted}
/> />
<Button <Button
+13 -13
View File
@@ -1,7 +1,8 @@
import { memo } from 'react' import { memo, useMemo } from 'react'
import type { Plant } from '@/lib/plants' import type { Plant } from '@/lib/plants'
import type { EditorPlanting } from '@/lib/plantings' import type { EditorPlanting } from '@/lib/plantings'
import { PlopMarker, SEMANTIC_FAR } from './PlopMarker' import { PlopMarker, SEMANTIC_FAR } from './PlopMarker'
import { DIMMED_OPACITY, objectTransform } from './shared'
import type { EditorObject } from './types' import type { EditorObject } from './types'
/** The most common plant color among an object's plops (for the far-zoom tint). */ /** The most common plant color among an object's plops (for the far-zoom tint). */
@@ -45,12 +46,16 @@ export const PlopLayer = memo(function PlopLayer({
selectedPlantingId: number | null selectedPlantingId: number | null
onSelectPlop: (id: number) => void onSelectPlop: (id: number) => void
}) { }) {
Review

🟠 PlopLayer recomputes full plop grouping + dominant-color scan every frame during any object drag or zoom, not just plop changes

performance · flagged by 2 models

  • web/src/editor/PlopLayer.tsx:48-53 (byObject grouping) and :64 (dominantColor call) — PlopLayer is wrapped in memo(), but its objects and scale props are not stable across common editor gestures, so the memo rarely pays off: - Any object drag/resize/rotate: SelectionOverlay.tsx:81 calls setLiveObject(onMove(ev)) on every pointermove, and GardenCanvas.tsx:95-98's rendered (passed to PlopLayer as objects, GardenCanvas.tsx:184) is a useMemo keyed on `live…

🪰 Gadfly · advisory

🟠 **PlopLayer recomputes full plop grouping + dominant-color scan every frame during any object drag or zoom, not just plop changes** _performance · flagged by 2 models_ - **`web/src/editor/PlopLayer.tsx:48-53`** (byObject grouping) and **`:64`** (`dominantColor` call) — `PlopLayer` is wrapped in `memo()`, but its `objects` and `scale` props are not stable across common editor gestures, so the memo rarely pays off: - **Any object drag/resize/rotate**: `SelectionOverlay.tsx:81` calls `setLiveObject(onMove(ev))` on every pointermove, and `GardenCanvas.tsx:95-98`'s `rendered` (passed to `PlopLayer` as `objects`, `GardenCanvas.tsx:184`) is a `useMemo` keyed on `live… <sub>🪰 Gadfly · advisory</sub>
const byObject = new Map<number, EditorPlanting[]>() // Group plops by object once per plops change (not on every pan/zoom frame).
for (const p of plantings) { const byObject = useMemo(() => {
const arr = byObject.get(p.objectId) const m = new Map<number, EditorPlanting[]>()
if (arr) arr.push(p) for (const p of plantings) {
else byObject.set(p.objectId, [p]) const arr = m.get(p.objectId)
} if (arr) arr.push(p)
else m.set(p.objectId, [p])
}
return m
}, [plantings])
const far = scale < SEMANTIC_FAR const far = scale < SEMANTIC_FAR
return ( return (
1
@@ -63,11 +68,7 @@ export const PlopLayer = memo(function PlopLayer({
const halfH = o.heightCm / 2 const halfH = o.heightCm / 2
Review

🟡 Object-local transform string duplicated across 5 files; extract a shared helper

maintainability · flagged by 2 models

  • web/src/editor/PlopLayer.tsx:69, web/src/editor/PlopMarker.tsx:49, web/src/editor/GardenCanvas.tsx:18 — Three separate dim-opacity constants (0.4, 0.3, 0.35) for the same visual concept. The PR introduced DIMMED_OPACITY in GardenCanvas but PlopLayer hardcoded 0.4 instead of reusing it (and PlopMarker's 0.3 is dead anyway). Consolidate to one exported constant.

🪰 Gadfly · advisory

🟡 **Object-local transform string duplicated across 5 files; extract a shared helper** _maintainability · flagged by 2 models_ - **`web/src/editor/PlopLayer.tsx:69`, `web/src/editor/PlopMarker.tsx:49`, `web/src/editor/GardenCanvas.tsx:18`** — Three separate dim-opacity constants (`0.4`, `0.3`, `0.35`) for the same visual concept. The PR introduced `DIMMED_OPACITY` in GardenCanvas but PlopLayer hardcoded `0.4` instead of reusing it (and PlopMarker's `0.3` is dead anyway). Consolidate to one exported constant. <sub>🪰 Gadfly · advisory</sub>
const tint = far ? dominantColor(plops, plantsById) : null const tint = far ? dominantColor(plops, plantsById) : null
return ( return (
<g <g key={o.id} transform={objectTransform(o)} opacity={dimmed ? DIMMED_OPACITY : 1}>
key={o.id}
transform={`translate(${o.xCm} ${o.yCm}) rotate(${o.rotationDeg})`}
opacity={dimmed ? 0.4 : 1}
>
{tint && {tint &&
(o.shape === 'circle' ? ( (o.shape === 'circle' ? (
<ellipse cx={0} cy={0} rx={halfW} ry={halfH} fill={tint} fillOpacity={0.5} pointerEvents="none" /> <ellipse cx={0} cy={0} rx={halfW} ry={halfH} fill={tint} fillOpacity={0.5} pointerEvents="none" />
@@ -81,7 +82,6 @@ export const PlopLayer = memo(function PlopLayer({
plant={plantsById.get(p.plantId)} plant={plantsById.get(p.plantId)}
scale={scale} scale={scale}
selected={p.id === selectedPlantingId} selected={p.id === selectedPlantingId}
Review

🟠 PlopMarker dimmed prop is always false and redundant with parent group opacity

maintainability · flagged by 2 models

  • web/src/editor/PlopLayer.tsx:84dimmed={false} is hardcoded for every PlopMarker, but PlopMarker declares dimmed: boolean as a required prop and branches on it (PlopMarker.tsx:49). Dead parameter: the layer already applies dim opacity at the group level (PlopLayer.tsx:69, opacity={dimmed ? 0.4 : 1}), so the marker-level dimmed can never be true. Grep confirms no caller ever passes true. Either drop the prop from PlopMarker or pass the real per-plop dim state.

🪰 Gadfly · advisory

🟠 **PlopMarker dimmed prop is always false and redundant with parent group opacity** _maintainability · flagged by 2 models_ - **`web/src/editor/PlopLayer.tsx:84`** — `dimmed={false}` is hardcoded for every `PlopMarker`, but `PlopMarker` declares `dimmed: boolean` as a required prop and branches on it (`PlopMarker.tsx:49`). Dead parameter: the layer already applies dim opacity at the group level (`PlopLayer.tsx:69`, `opacity={dimmed ? 0.4 : 1}`), so the marker-level `dimmed` can never be `true`. Grep confirms no caller ever passes `true`. Either drop the prop from `PlopMarker` or pass the real per-plop dim state. <sub>🪰 Gadfly · advisory</sub>
dimmed={false}
onSelect={onSelectPlop} onSelect={onSelectPlop}
/> />
))} ))}
+8 -13
View File
@@ -1,6 +1,7 @@
import { memo, type PointerEvent } from 'react' import { memo, type PointerEvent } from 'react'
import type { Plant } from '@/lib/plants' import type { Plant } from '@/lib/plants'
import { effectiveCount, type EditorPlanting } from '@/lib/plantings' import { computeDerivedCount, type EditorPlanting } from '@/lib/plantings'
import { SELECT_COLOR } from './shared'
// Semantic-zoom thresholds in px/cm (DESIGN § Editor / rendering), tuned by feel: // Semantic-zoom thresholds in px/cm (DESIGN § Editor / rendering), tuned by feel:
// < FAR — flat color patch, no text ("what's planted where" at a glance) // < FAR — flat color patch, no text ("what's planted where" at a glance)
@@ -9,34 +10,33 @@ import { effectiveCount, type EditorPlanting } from '@/lib/plantings'
export const SEMANTIC_FAR = 0.75 export const SEMANTIC_FAR = 0.75
export const SEMANTIC_NEAR = 3 export const SEMANTIC_NEAR = 3
const SELECT_COLOR = '#2f7a3e'
const DEFAULT_COLOR = '#6aa84f' const DEFAULT_COLOR = '#6aa84f'
/** /**
* One plop, rendered in its parent object's local frame (the caller wraps it in * One plop, rendered in its parent object's local frame (the caller wraps it in
* the object's translate+rotate group, so the plop tracks the bed). A circle in * the object's translate+rotate group, so the plop tracks the bed). A circle in
* the plant's color; emoji and name/count fade in with zoom. memo'd so a pan/zoom * the plant's color; emoji and name/count fade in with zoom. The count is
* that only changes the world transform doesn't re-render every plop. * computed live from the current radius so it updates while resizing. memo'd so a
* pan/zoom that only changes the world transform doesn't re-render every plop.
*/ */
export const PlopMarker = memo(function PlopMarker({ export const PlopMarker = memo(function PlopMarker({
plop, plop,
plant, plant,
scale, scale,
selected, selected,
dimmed,
onSelect, onSelect,
Review

🟡 Dead dimmed prop in PlopMarker

maintainability · flagged by 1 model

  • PlopMarker carries a dead dimmed prop (web/src/editor/PlopMarker.tsx:27). The only caller (PlopLayer) always passes dimmed={false} and handles object-level dimming via the parent <g> opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers.

🪰 Gadfly · advisory

🟡 **Dead dimmed prop in PlopMarker** _maintainability · flagged by 1 model_ - **`PlopMarker` carries a dead `dimmed` prop** (`web/src/editor/PlopMarker.tsx:27`). The only caller (`PlopLayer`) always passes `dimmed={false}` and handles object-level dimming via the parent `<g>` opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers. <sub>🪰 Gadfly · advisory</sub>
}: { }: {
plop: EditorPlanting plop: EditorPlanting
plant?: Plant plant?: Plant
scale: number scale: number
selected: boolean selected: boolean
dimmed: boolean
onSelect: (id: number) => void onSelect: (id: number) => void
}) { }) {
const color = plant?.color ?? DEFAULT_COLOR const color = plant?.color ?? DEFAULT_COLOR
const r = Math.max(0, plop.radiusCm) const r = Math.max(0, plop.radiusCm)
const showIcon = scale >= SEMANTIC_FAR && !!plant?.icon const showIcon = scale >= SEMANTIC_FAR && !!plant?.icon
const showText = scale >= SEMANTIC_NEAR && !!plant const showText = scale >= SEMANTIC_NEAR && !!plant
const count = plop.count ?? (plant ? computeDerivedCount(plop.radiusCm, plant.spacingCm) : plop.derivedCount)
function down(e: PointerEvent) { function down(e: PointerEvent) {
e.stopPropagation() e.stopPropagation()
@@ -44,12 +44,7 @@ export const PlopMarker = memo(function PlopMarker({
} }
return ( return (
<g <g transform={`translate(${plop.xCm} ${plop.yCm})`} onPointerDown={down} style={{ cursor: 'pointer' }}>
transform={`translate(${plop.xCm} ${plop.yCm})`}
opacity={dimmed ? 0.3 : 1}
onPointerDown={down}
style={{ cursor: 'pointer' }}
>
<circle <circle
cx={0} cx={0}
cy={0} cy={0}
@@ -85,7 +80,7 @@ export const PlopMarker = memo(function PlopMarker({
paintOrder="stroke" paintOrder="stroke"
style={{ pointerEvents: 'none', userSelect: 'none' }} style={{ pointerEvents: 'none', userSelect: 'none' }}
> >
{plant!.name} · {effectiveCount(plop)} {plant!.name} · {count}
</text> </text>
)} )}
</g> </g>
1
+2 -4
View File
@@ -2,13 +2,11 @@ import { useEffect, useRef, type PointerEvent as ReactPointerEvent, type RefObje
import { screenToWorld, worldToLocal, type Point } from '@/lib/geometry' import { screenToWorld, worldToLocal, type Point } from '@/lib/geometry'
import { useUpdatePlanting } from '@/lib/objects' import { useUpdatePlanting } from '@/lib/objects'
import type { EditorPlanting } from '@/lib/plantings' import type { EditorPlanting } from '@/lib/plantings'
import { HANDLE_PX, MIN_RADIUS_CM, SELECT_COLOR, objectTransform } from './shared'
import { useEditorStore } from './store' import { useEditorStore } from './store'
import type { EditorObject } from './types' import type { EditorObject } from './types'
const HANDLE_PX = 12
const MIN_RADIUS_CM = 1
const MAX_RADIUS_CM = 10_000 const MAX_RADIUS_CM = 10_000
const SELECT_COLOR = '#2f7a3e'
/** /**
Review

🟡 SELECT_COLOR hex constant duplicated across SelectionOverlay/PlopMarker/PlopOverlay instead of shared

maintainability · flagged by 3 models

  • web/src/editor/PlopOverlay.tsx:11 / web/src/editor/PlopMarker.tsx:12const SELECT_COLOR = '#2f7a3e' is now defined identically in three places: the pre-existing SelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. editor/colors.ts) or exporting it from SelectionOverlay.tsx. - **web/src/editor/PlopInspector.tsx:10 and `w…

🪰 Gadfly · advisory

🟡 **SELECT_COLOR hex constant duplicated across SelectionOverlay/PlopMarker/PlopOverlay instead of shared** _maintainability · flagged by 3 models_ - **`web/src/editor/PlopOverlay.tsx:11` / `web/src/editor/PlopMarker.tsx:12`** — `const SELECT_COLOR = '#2f7a3e'` is now defined identically in three places: the pre-existing `SelectionOverlay.tsx:11`, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. `editor/colors.ts`) or exporting it from `SelectionOverlay.tsx`. - **`web/src/editor/PlopInspector.tsx:10` and `w… <sub>🪰 Gadfly · advisory</sub>
* Move/resize handles for the selected plop, rendered inside its parent object's * Move/resize handles for the selected plop, rendered inside its parent object's
3
@@ -127,7 +125,7 @@ export function PlopOverlay({
const r = plop.radiusCm const r = plop.radiusCm
return ( return (
<g transform={`translate(${object.xCm} ${object.yCm}) rotate(${object.rotationDeg})`}> <g transform={objectTransform(object)}>
{/* Transparent body: drag to move (at least handle-sized so tiny plops stay grabbable). */} {/* Transparent body: drag to move (at least handle-sized so tiny plops stay grabbable). */}
<circle <circle
cx={plop.xCm} cx={plop.xCm}
+2 -3
View File
@@ -1,14 +1,13 @@
import { useEffect, useRef, type PointerEvent as ReactPointerEvent, type RefObject } from 'react' import { useEffect, useRef, type PointerEvent as ReactPointerEvent, type RefObject } from 'react'
import { localToWorld, screenToWorld, worldToLocal, type Point } from '@/lib/geometry' import { localToWorld, screenToWorld, worldToLocal, type Point } from '@/lib/geometry'
import { useUpdateObject } from '@/lib/objects' import { useUpdateObject } from '@/lib/objects'
import { HANDLE_PX, SELECT_COLOR, objectTransform } from './shared'
import { useEditorStore } from './store' import { useEditorStore } from './store'
import type { EditorObject } from './types' import type { EditorObject } from './types'
const HANDLE_PX = 12 // on-screen size of the resize handles
const ROTATE_OFFSET_PX = 28 // distance of the rotate knob above the object const ROTATE_OFFSET_PX = 28 // distance of the rotate knob above the object
const MIN_OBJ_CM = 1 // smallest allowed dimension const MIN_OBJ_CM = 1 // smallest allowed dimension
const ROTATE_SNAP_DEG = 15 const ROTATE_SNAP_DEG = 15
const SELECT_COLOR = '#2f7a3e'
const corners: [number, number][] = [ const corners: [number, number][] = [
[-1, -1], [-1, -1],
@@ -162,7 +161,7 @@ export function SelectionOverlay({
} }
return ( return (
<g transform={`translate(${object.xCm} ${object.yCm}) rotate(${object.rotationDeg})`}> <g transform={objectTransform(object)}>
{/* Transparent body: drag to move. */} {/* Transparent body: drag to move. */}
{object.shape === 'circle' ? ( {object.shape === 'circle' ? (
<ellipse cx={0} cy={0} rx={halfW} ry={halfH} fill="transparent" pointerEvents="all" style={{ cursor: 'move' }} onPointerDown={startMove} /> <ellipse cx={0} cy={0} rx={halfW} ry={halfH} fill="transparent" pointerEvents="all" style={{ cursor: 'move' }} onPointerDown={startMove} />
+14
View File
@@ -0,0 +1,14 @@
// Shared editor UI constants + tiny helpers used across the object/plop markers
// and overlays, so colors, handle sizes, and the object-local transform stay in
// one place instead of drifting between files.
export const SELECT_COLOR = '#2f7a3e' // selection stroke/handles
export const HANDLE_PX = 12 // on-screen size of a drag/resize handle
export const MIN_RADIUS_CM = 1 // smallest plop radius
export const DIMMED_OPACITY = 0.4 // non-focused objects/plops in focus mode
/** SVG transform placing content in an object's local frame: its center point
* then its clockwise rotation. Plops and overlays render inside this. */
export function objectTransform(o: { xCm: number; yCm: number; rotationDeg: number }): string {
return `translate(${o.xCm} ${o.yCm}) rotate(${o.rotationDeg})`
}
+13 -1
View File
2
@@ -280,7 +280,18 @@ export function useRemovePlanting(gardenId: number) {
}, },
onError: (err, _vars, ctx) => { onError: (err, _vars, ctx) => {
Review

🟠 useRemovePlanting onError doesn't adopt the 409 conflict's current row, so a stale-version remove loops on 409 and the plop can't be removed until an unrelated refetch

error-handling · flagged by 1 model

  • useRemovePlanting doesn't reconcile a 409 conflict, leaving the user stuck. web/src/lib/objects.ts:281-284: the onError only restores ctx.prev and shows a generic toast. useUpdatePlanting (lines 250-257) instead calls conflictPlanting(err) and, on a conflict, writes the server's current row into the cache so the next edit carries a fresh version. useRemovePlanting lacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale)…

🪰 Gadfly · advisory

🟠 **useRemovePlanting onError doesn't adopt the 409 conflict's current row, so a stale-version remove loops on 409 and the plop can't be removed until an unrelated refetch** _error-handling · flagged by 1 model_ - **`useRemovePlanting` doesn't reconcile a 409 conflict, leaving the user stuck.** `web/src/lib/objects.ts:281-284`: the `onError` only restores `ctx.prev` and shows a generic toast. `useUpdatePlanting` (lines 250-257) instead calls `conflictPlanting(err)` and, on a conflict, writes the server's `current` row into the cache so the next edit carries a fresh `version`. `useRemovePlanting` lacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale)… <sub>🪰 Gadfly · advisory</sub>
if (ctx?.prev) qc.setQueryData(fullKey(gardenId), ctx.prev) if (ctx?.prev) qc.setQueryData(fullKey(gardenId), ctx.prev)
toast.error(objectErrorMessage(err, 'Could not remove that plant.')) // On a 409, adopt the server's current row so the plop reappears with a
// fresh version — otherwise a retry keeps 409-ing on the stale version.
const current = conflictPlanting(err)
if (current) {
patchFullCache(qc, gardenId, (full) => ({
...full,
plantings: full.plantings.map((p) => (p.id === current.id ? current : p)),
}))
toast.error('That plant was updated elsewhere — try removing it again.')
} else {
toast.error(objectErrorMessage(err, 'Could not remove that plant.'))
}
}, },
}) })
} }
1
@@ -307,6 +318,7 @@ function applyPlantingPatch(p: ServerPlanting, patch: PlantingPatch): ServerPlan
...(patch.count !== undefined ? { count: patch.count } : {}), ...(patch.count !== undefined ? { count: patch.count } : {}),
...(patch.label !== undefined ? { label: patch.label } : {}), ...(patch.label !== undefined ? { label: patch.label } : {}),
...(patch.plantedAt !== undefined ? { plantedAt: patch.plantedAt } : {}), ...(patch.plantedAt !== undefined ? { plantedAt: patch.plantedAt } : {}),
...(patch.removedAt !== undefined ? { removedAt: patch.removedAt } : {}),
} }
} }
+14 -6
View File
@@ -32,7 +32,6 @@ export function GardenEditorPage() {
const setArmedPlant = useEditorStore((s) => s.setArmedPlant) const setArmedPlant = useEditorStore((s) => s.setArmedPlant)
const setArmedKind = useEditorStore((s) => s.setArmedKind) const setArmedKind = useEditorStore((s) => s.setArmedKind)
const setLiveObject = useEditorStore((s) => s.setLiveObject) const setLiveObject = useEditorStore((s) => s.setLiveObject)
const livePlanting = useEditorStore((s) => s.livePlanting)
const setLivePlanting = useEditorStore((s) => s.setLivePlanting) const setLivePlanting = useEditorStore((s) => s.setLivePlanting)
const updatePlanting = useUpdatePlanting(gid) const updatePlanting = useUpdatePlanting(gid)
1
@@ -75,6 +74,14 @@ export function GardenEditorPage() {
navigate({ search: (prev) => ({ ...prev, focus: focusedObjectId ?? undefined }), replace: true }) navigate({ search: (prev) => ({ ...prev, focus: focusedObjectId ?? undefined }), replace: true })
}, [focusedObjectId, navigate]) }, [focusedObjectId, navigate])
// If the focused object vanished — deleted, or a stale ?focus id on load — leave
// focus mode so the canvas isn't stuck dimmed with no way out.
useEffect(() => {
if (focusedObjectId != null && full.data && !objects.some((o) => o.id === focusedObjectId)) {
setFocusedObject(null)
}
}, [focusedObjectId, objects, full.data, setFocusedObject])
const exitFocus = () => { const exitFocus = () => {
setFocusedObject(null) setFocusedObject(null)
setArmedPlant(null) setArmedPlant(null)
@@ -116,9 +123,8 @@ export function GardenEditorPage() {
const selectedObject = objects.find((o) => o.id === selectedId) ?? null const selectedObject = objects.find((o) => o.id === selectedId) ?? null
const focusedObject = focusedObjectId != null ? objects.find((o) => o.id === focusedObjectId) ?? null : null const focusedObject = focusedObjectId != null ? objects.find((o) => o.id === focusedObjectId) ?? null : null
const rawSelectedPlop = plantings.find((p) => p.id === selectedPlantingId) ?? null // Committed selected plop; PlopInspector applies live drag geometry itself.
const selectedPlop = const selectedPlop = plantings.find((p) => p.id === selectedPlantingId) ?? null
rawSelectedPlop && livePlanting?.id === rawSelectedPlop.id ? livePlanting : rawSelectedPlop
function onPickPlant(plantId: number) { function onPickPlant(plantId: number) {
const plant = plantsById.get(plantId) const plant = plantsById.get(plantId)
@@ -149,7 +155,9 @@ export function GardenEditorPage() {
<span className="max-w-[8rem] truncate text-muted"> <span className="max-w-[8rem] truncate text-muted">
{focusedObject.name || kindDef(focusedObject.kind)?.label || 'Object'} {focusedObject.name || kindDef(focusedObject.kind)?.label || 'Object'}
</span> </span>
{armedPlant ? ( {!focusedObject.plantable ? (
<span className="text-xs text-muted">Not plantable</span>
) : armedPlant ? (
<Button variant="ghost" className="px-2 py-1 text-xs" onClick={() => setArmedPlant(null)}> <Button variant="ghost" className="px-2 py-1 text-xs" onClick={() => setArmedPlant(null)}>
Placing {armedPlant.icon} Done Placing {armedPlant.icon} Done
</Button> </Button>
@@ -160,7 +168,7 @@ export function GardenEditorPage() {
)} )}
</div> </div>
)} )}
<GardenCanvas garden={garden} objects={objects} plantings={plantings} plants={plants} /> <GardenCanvas garden={garden} objects={objects} plantings={plantings} plantsById={plantsById} />
</div> </div>
{(selectedObject || selectedPlop) && ( {(selectedObject || selectedPlop) && (