Field editor: palette, select/move/resize/rotate, inspector, optimistic sync (#11) #30

Merged
steve merged 2 commits from phase-3-field-editor into main 2026-07-19 01:01:39 +00:00
7 changed files with 140 additions and 75 deletions
Showing only changes of commit 2cde8a24f5 - Show all commits
+8 -7
View File
@@ -29,23 +29,24 @@ export const toast = {
error: (m: string) => useToastStore.getState().push(m, 'error'), error: (m: string) => useToastStore.getState().push(m, 'error'),
} }
function ToastItem({ toast }: { toast: Toast }) { // Param is `item`, not `toast`, so it doesn't shadow the module's `toast` export.
Outdated
Review

🟡 ToastItem prop toast shadows the same-file exported toast helper

maintainability · flagged by 2 models

  • web/src/components/ui/toast.tsx:32ToastItem prop toast shadows the imported/exported toast helper. The component parameter { toast } (line 32) shadows the module-level export const toast = {...} (lines 27-30) within the component body. It works (the component only reads toast.id/toast.tone/toast.message), but shadowing a same-file exported symbol is a readability trap — rename the prop to data or t.

🪰 Gadfly · advisory

🟡 **ToastItem prop `toast` shadows the same-file exported `toast` helper** _maintainability · flagged by 2 models_ - **`web/src/components/ui/toast.tsx:32` — `ToastItem` prop `toast` shadows the imported/exported `toast` helper.** The component parameter `{ toast }` (line 32) shadows the module-level `export const toast = {...}` (lines 27-30) within the component body. It works (the component only reads `toast.id`/`toast.tone`/`toast.message`), but shadowing a same-file exported symbol is a readability trap — rename the prop to `data` or `t`. <sub>🪰 Gadfly · advisory</sub>
function ToastItem({ item }: { item: Toast }) {
const dismiss = useToastStore((s) => s.dismiss) const dismiss = useToastStore((s) => s.dismiss)
useEffect(() => { useEffect(() => {
const t = setTimeout(() => dismiss(toast.id), 4000) const t = setTimeout(() => dismiss(item.id), 4000)
return () => clearTimeout(t) return () => clearTimeout(t)
}, [toast.id, dismiss]) }, [item.id, dismiss])
return ( return (
<div <div
role={toast.tone === 'error' ? 'alert' : 'status'} role={item.tone === 'error' ? 'alert' : 'status'}
className={cn( className={cn(
'pointer-events-auto rounded-md border px-3 py-2 text-sm shadow-md', 'pointer-events-auto rounded-md border px-3 py-2 text-sm shadow-md',
toast.tone === 'error' item.tone === 'error'
? 'border-red-500/40 bg-red-500/10 text-red-700 dark:text-red-300' ? 'border-red-500/40 bg-red-500/10 text-red-700 dark:text-red-300'
: 'border-border bg-surface text-fg', : 'border-border bg-surface text-fg',
)} )}
> >
{toast.message} {item.message}
</div> </div>
) )
} }
@@ -57,7 +58,7 @@ export function Toaster() {
return ( return (
<div className="pointer-events-none fixed bottom-4 left-1/2 z-[60] flex -translate-x-1/2 flex-col items-center gap-2"> <div className="pointer-events-none fixed bottom-4 left-1/2 z-[60] flex -translate-x-1/2 flex-col items-center gap-2">
{toasts.map((t) => ( {toasts.map((t) => (
<ToastItem key={t.id} toast={t} /> <ToastItem key={t.id} item={t} />
))} ))}
</div> </div>
) )
+9 -14
View File
@@ -12,14 +12,6 @@ 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
// Kinds that render beneath beds by default; explicit z_index still wins.
const UNDER_KINDS = new Set(['path', 'structure', 'in_ground'])
function defaultZForKind(kind: string): number {
if (UNDER_KINDS.has(kind)) return 0
if (kind === 'tree') return 2
return 1
}
/** /**
Outdated
Review

🟠 defaultZForKind hard-codes kind strings that must stay in sync with kinds.ts

maintainability · flagged by 3 models

  • web/src/editor/SelectionOverlay.tsx:138-150 — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. - web/src/lib/objects.ts:188-202applyServerPatch manually spreads each of the 10 patchable fields with !== undefined guards. Adding a new patchable field requires touching this function,…

🪰 Gadfly · advisory

🟠 **defaultZForKind hard-codes kind strings that must stay in sync with kinds.ts** _maintainability · flagged by 3 models_ - **`web/src/editor/SelectionOverlay.tsx:138-150`** — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (`<ShapeBody shape={…} halfW={…} halfH={…} {...props} />`) so the shape fork lives in one place. - **`web/src/lib/objects.ts:188-202`** — `applyServerPatch` manually spreads each of the 10 patchable fields with `!== undefined` guards. Adding a new patchable field requires touching this function,… <sub>🪰 Gadfly · advisory</sub>
* The editor canvas. Pan/zoom/pinch (from useViewport), tap-to-place from the * The editor canvas. Pan/zoom/pinch (from useViewport), tap-to-place from the
* armed palette kind, click to select, and — for the selected object — * armed palette kind, click to select, and — for the selected object —
@@ -78,11 +70,14 @@ export function GardenCanvas({
const cellPx = GRID_CM * viewport.scale const cellPx = GRID_CM * viewport.scale
const gridOpacity = Math.max(0, Math.min(GRID_MAX_OPACITY, ((cellPx - GRID_MIN_CELL_PX) / GRID_MIN_CELL_PX) * GRID_MAX_OPACITY)) const gridOpacity = Math.max(0, Math.min(GRID_MAX_OPACITY, ((cellPx - GRID_MIN_CELL_PX) / GRID_MIN_CELL_PX) * GRID_MAX_OPACITY))
Outdated
Review

🔴 fittedForRef guard prevents re-framing when focusId changes for same garden

correctness · flagged by 2 models

  • web/src/editor/GardenCanvas.tsx:74-82 — The fittedForRef.current !== garden.id guard means the canvas frames the garden (or a ?focus= object) exactly once per garden mount. If the focusId search param changes client-side without remounting the page, the viewport does not re-fit to the new object because the guard only tracks garden.id, not the focus target. Track the last focused id and re-frame when it changes.

🪰 Gadfly · advisory

🔴 **fittedForRef guard prevents re-framing when focusId changes for same garden** _correctness · flagged by 2 models_ - `web/src/editor/GardenCanvas.tsx:74-82` — The `fittedForRef.current !== garden.id` guard means the canvas frames the garden (or a `?focus=` object) exactly once per garden mount. If the `focusId` search param changes client-side without remounting the page, the viewport does **not** re-fit to the new object because the guard only tracks `garden.id`, not the focus target. Track the last focused id and re-frame when it changes. <sub>🪰 Gadfly · advisory</sub>
// Merge the in-flight live geometry over the server objects, then z-order. // Sort once by z-order; only re-sorts when the server objects change.
const rendered = useMemo(() => { const sorted = useMemo(() => [...objects].sort((a, b) => a.zIndex - b.zIndex), [objects])
const merged = liveObject ? objects.map((o) => (o.id === liveObject.id ? liveObject : o)) : objects // Merge the in-flight live geometry over the sorted list. A gesture never
return [...merged].sort((a, b) => a.zIndex - b.zIndex) // changes z, so this stays a cheap O(n) map on every pointermove — no re-sort.
}, [objects, liveObject]) const rendered = useMemo(
() => (liveObject ? sorted.map((o) => (o.id === liveObject.id ? liveObject : o)) : sorted),
[sorted, liveObject],
)
const selected = rendered.find((o) => o.id === selectedId) ?? null const selected = rendered.find((o) => o.id === selectedId) ?? null
Review

🟠 Full object array re-mapped and re-sorted on every drag pointermove instead of only repositioning the dragged object

maintainability, performance · flagged by 5 models

  • web/src/editor/GardenCanvas.tsx:82-85rendered re-maps and re-sorts the full objects array on every liveObject change (every pointermove during a drag, per SelectionOverlay's setLiveObject calls). Confirmed the useMemo dependency array is [objects, liveObject] (line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmed GardenEditorPage.tsx:51 (`full.data.objects.map(toEditorO…

🪰 Gadfly · advisory

🟠 **Full object array re-mapped and re-sorted on every drag pointermove instead of only repositioning the dragged object** _maintainability, performance · flagged by 5 models_ - **`web/src/editor/GardenCanvas.tsx:82-85`** — `rendered` re-maps and re-sorts the full `objects` array on every `liveObject` change (every pointermove during a drag, per `SelectionOverlay`'s `setLiveObject` calls). Confirmed the `useMemo` dependency array is `[objects, liveObject]` (line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmed `GardenEditorPage.tsx:51` (`full.data.objects.map(toEditorO… <sub>🪰 Gadfly · advisory</sub>
// A pointerdown reaching the svg is empty space: place the armed kind there, or // A pointerdown reaching the svg is empty space: place the armed kind there, or
@@ -107,7 +102,7 @@ export function GardenCanvas({
yCm: world.y, yCm: world.y,
widthCm: def.widthCm, widthCm: def.widthCm,
heightCm: def.heightCm, heightCm: def.heightCm,
zIndex: defaultZForKind(def.kind), zIndex: def.defaultZ,
}, },
{ onSuccess: (o) => select(o.id) }, { onSuccess: (o) => select(o.id) },
) )
2
+34 -8
View File
@@ -3,11 +3,19 @@ import { Button } from '@/components/ui/Button'
import { TextField } from '@/components/ui/TextField' import { TextField } from '@/components/ui/TextField'
import { TextArea } from '@/components/ui/TextArea' import { TextArea } from '@/components/ui/TextArea'
import { useDeleteObject, useUpdateObject } from '@/lib/objects' import { useDeleteObject, useUpdateObject } from '@/lib/objects'
import { cmFromDisplay, dimensionUnitLabel, displayFromCm, type UnitPref } from '@/lib/units' import {
cmFromDisplay,
dimensionUnitLabel,
displayFromCm,
MIN_DIMENSION_CM,
type UnitPref,
} from '@/lib/units'
import { kindDef } from './kinds' import { kindDef } from './kinds'
import { useEditorStore } from './store' import { useEditorStore } from './store'
import type { EditorObject } from './types' import type { EditorObject } from './types'
const DEFAULT_COLOR = '#8a8a8a'
/** /**
* Property panel for the selected object. Each field commits a PATCH on * Property panel for the selected object. Each field commits a PATCH on
* blur/change (carrying the object's version); dimensions and position are shown * blur/change (carrying the object's version); dimensions and position are shown
1
@@ -35,6 +43,7 @@ export function Inspector({
const [x, setX] = useState(String(displayFromCm(object.xCm, unit))) const [x, setX] = useState(String(displayFromCm(object.xCm, unit)))
const [y, setY] = useState(String(displayFromCm(object.yCm, unit))) const [y, setY] = useState(String(displayFromCm(object.yCm, unit)))
Review

🟠 Re-sync skips all inspector fields when any field has focus

error-handling · flagged by 1 model

  • web/src/editor/Inspector.tsx:44-53 — The re-sync useEffect aborts entirely when any field inside the inspector has focus (rootRef.current?.contains(document.activeElement)). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field.

🪰 Gadfly · advisory

🟠 **Re-sync skips all inspector fields when any field has focus** _error-handling · flagged by 1 model_ - **`web/src/editor/Inspector.tsx:44-53`** — The re-sync `useEffect` aborts entirely when *any* field inside the inspector has focus (`rootRef.current?.contains(document.activeElement)`). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field. <sub>🪰 Gadfly · advisory</sub>
const [rotation, setRotation] = useState(String(Math.round(object.rotationDeg))) const [rotation, setRotation] = useState(String(Math.round(object.rotationDeg)))
const [color, setColor] = useState(object.color ?? DEFAULT_COLOR)
const [confirmingDelete, setConfirmingDelete] = useState(false) const [confirmingDelete, setConfirmingDelete] = useState(false)
const rootRef = useRef<HTMLDivElement>(null) const rootRef = useRef<HTMLDivElement>(null)
1
@@ -50,15 +59,22 @@ export function Inspector({
setX(String(displayFromCm(object.xCm, unit))) setX(String(displayFromCm(object.xCm, unit)))
setY(String(displayFromCm(object.yCm, unit))) setY(String(displayFromCm(object.yCm, unit)))
setRotation(String(Math.round(object.rotationDeg))) setRotation(String(Math.round(object.rotationDeg)))
}, [object.version, object.widthCm, object.heightCm, object.xCm, object.yCm, object.rotationDeg, object.name, object.notes, unit]) setColor(object.color ?? DEFAULT_COLOR)
Outdated
Review

🟠 Blur of an unchanged dimension/position/rotation field fires a spurious PATCH due to lossy unit round-trip (e.g. 100cm -> 3.3ft -> 101cm), mutating the object and bumping version on a no-op focus/blur

correctness · flagged by 2 models

  • web/src/editor/Inspector.tsx:62 — spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift. commitDim runs on every blur and compares cmFromDisplay(v, unit) to current with no dirty guard. Because displayFromCm rounds (imperial → 0.1 ft, metric → 2 dp cm) and cmFromDisplay re-quantizes to whole cm, the round trip is lossy. Verified: 100 cm → displayFromCm gives 3.3 ft → cmFromDisplay(3.3,'imperial') = round(3.3*12*2.54) = 101 cm, so…

🪰 Gadfly · advisory

🟠 **Blur of an unchanged dimension/position/rotation field fires a spurious PATCH due to lossy unit round-trip (e.g. 100cm -> 3.3ft -> 101cm), mutating the object and bumping version on a no-op focus/blur** _correctness · flagged by 2 models_ - **`web/src/editor/Inspector.tsx:62` — spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift.** `commitDim` runs on every blur and compares `cmFromDisplay(v, unit)` to `current` with no dirty guard. Because `displayFromCm` rounds (imperial → 0.1 ft, metric → 2 dp cm) and `cmFromDisplay` re-quantizes to whole cm, the round trip is lossy. Verified: 100 cm → `displayFromCm` gives `3.3` ft → `cmFromDisplay(3.3,'imperial')` = `round(3.3*12*2.54)` = **101 cm**, so… <sub>🪰 Gadfly · advisory</sub>
}, [object.version, object.widthCm, object.heightCm, object.xCm, object.yCm, object.rotationDeg, object.name, object.notes, object.color, unit])
const patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) => const patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) =>
update.mutate({ id: object.id, version: object.version, ...fields }) update.mutate({ id: object.id, version: object.version, ...fields })
const commitDim = (raw: string, current: number, apply: (cm: number) => void) => { // Commit a dimension/position field. `positive` gates width/height (must be
// ≥ the server minimum) but not x/y, which may be zero or negative. Compare at
// display precision first so a blur without an edit doesn't fire a spurious
// PATCH from cm↔ft rounding drift (e.g. 240cm shows 7.9ft → 241cm).
const commitDim = (raw: string, current: number, apply: (cm: number) => void, positive = false) => {
const v = parseFloat(raw) const v = parseFloat(raw)
if (!Number.isFinite(v)) return if (!Number.isFinite(v)) return
if (v === displayFromCm(current, unit)) return
const cm = cmFromDisplay(v, unit) const cm = cmFromDisplay(v, unit)
if (positive && cm < MIN_DIMENSION_CM) return
if (cm !== current) apply(cm) if (cm !== current) apply(cm)
} }
1
@@ -95,7 +111,7 @@ export function Inspector({
step="any" step="any"
value={width} value={width}
onChange={(e) => setWidth(e.target.value)} onChange={(e) => setWidth(e.target.value)}
onBlur={() => commitDim(width, object.widthCm, (cm) => patch({ widthCm: cm }))} onBlur={() => commitDim(width, object.widthCm, (cm) => patch({ widthCm: cm }), true)}
/> />
<TextField <TextField
label={`Height (${u})`} label={`Height (${u})`}
@@ -105,7 +121,7 @@ export function Inspector({
step="any" step="any"
value={height} value={height}
onChange={(e) => setHeight(e.target.value)} onChange={(e) => setHeight(e.target.value)}
onBlur={() => commitDim(height, object.heightCm, (cm) => patch({ heightCm: cm }))} onBlur={() => commitDim(height, object.heightCm, (cm) => patch({ heightCm: cm }), true)}
/> />
<TextField <TextField
label={`X (${u})`} label={`X (${u})`}
1
@@ -151,13 +167,23 @@ export function Inspector({
<input <input
id="obj-color" id="obj-color"
type="color" type="color"
value={object.color ?? '#8a8a8a'} value={color}
Outdated
Review

🟠 Checkbox commits PATCH immediately onChange, rapid toggles self-conflict with version

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟠 **Checkbox commits PATCH immediately onChange, rapid toggles self-conflict with version** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
onChange={(e: ChangeEvent<HTMLInputElement>) => patch({ color: e.target.value })} // onChange fires continuously while dragging in the native picker;
// track it locally for the live swatch and commit one PATCH on blur.
onChange={(e: ChangeEvent<HTMLInputElement>) => setColor(e.target.value)}
onBlur={() => color !== (object.color ?? DEFAULT_COLOR) && patch({ color })}
className="h-9 w-14 cursor-pointer rounded-md border border-border bg-surface" className="h-9 w-14 cursor-pointer rounded-md border border-border bg-surface"
/> />
</div> </div>
{object.color && ( {object.color && (
<Button variant="ghost" className="px-2 py-1.5 text-xs" onClick={() => patch({ color: null })}> <Button
variant="ghost"
className="px-2 py-1.5 text-xs"
onClick={() => {
setColor(DEFAULT_COLOR)
patch({ color: null })
}}
>
Clear Clear
</Button> </Button>
)} )}
1
+56 -28
View File
@@ -1,4 +1,4 @@
import type { PointerEvent as ReactPointerEvent, 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 { useEditorStore } from './store' import { useEditorStore } from './store'
@@ -17,20 +17,6 @@ const corners: [number, number][] = [
[-1, 1], [-1, 1],
] ]
Review

🔴 Window pointer listeners leak when SelectionOverlay unmounts mid-drag

error-handling · flagged by 4 models

  • web/src/editor/SelectionOverlay.tsx:21-31trackPointer attaches window listeners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still on window. On pointerup the onEnd closure fires, calling setObjectDragging(false) and update.mutate(fields(final)) ag…

🪰 Gadfly · advisory

🔴 **Window pointer listeners leak when SelectionOverlay unmounts mid-drag** _error-handling · flagged by 4 models_ - **`web/src/editor/SelectionOverlay.tsx:21-31`** — `trackPointer` attaches `window` listeners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still on `window`. On `pointerup` the `onEnd` closure fires, calling `setObjectDragging(false)` and `update.mutate(fields(final))` ag… <sub>🪰 Gadfly · advisory</sub>
// Register window listeners for a pointer drag; cleaned up on up/cancel.
function trackPointer(onMove: (e: PointerEvent) => void, onEnd: () => void) {
const move = (e: PointerEvent) => onMove(e)
const end = () => {
window.removeEventListener('pointermove', move)
window.removeEventListener('pointerup', end)
window.removeEventListener('pointercancel', end)
onEnd()
}
window.addEventListener('pointermove', move)
window.addEventListener('pointerup', end)
window.addEventListener('pointercancel', end)
}
/** /**
* Handles for the selected object: a transparent body to move, four corner * Handles for the selected object: a transparent body to move, four corner
* handles to resize (opposite corner stays put, honoring rotation via the * handles to resize (opposite corner stays put, honoring rotation via the
@@ -52,47 +38,83 @@ export function SelectionOverlay({
const scale = useEditorStore((s) => s.viewport.scale) const scale = useEditorStore((s) => s.viewport.scale)
const update = useUpdateObject(gardenId) const update = useUpdateObject(gardenId)
// Detach for an in-flight gesture. If the overlay unmounts mid-drag (the
// object is deleted or deselected), this runs on unmount so window listeners
// don't leak and objectDragging can't stay stuck true (which would freeze pan).
const cleanupRef = useRef<(() => void) | null>(null)
useEffect(
() => () => {
cleanupRef.current?.()
cleanupRef.current = null
},
[],
)
const halfW = object.widthCm / 2 const halfW = object.widthCm / 2
const halfH = object.heightCm / 2 const halfH = object.heightCm / 2
const handleCm = HANDLE_PX / scale const handleCm = HANDLE_PX / scale
const rotateOffsetCm = ROTATE_OFFSET_PX / scale const rotateOffsetCm = ROTATE_OFFSET_PX / scale
Review

🟠 getBoundingClientRect() called on every pointermove during drags — forces layout reflow each move event (layout thrash)

maintainability, performance · flagged by 4 models

  • web/src/editor/SelectionOverlay.tsx:60-65getBoundingClientRect() called on every pointermove during a drag. pointerWorld reads svgRef.current?.getBoundingClientRect() and is invoked from the trackPointer move callback (line 78) on every pointer event. getBoundingClientRect forces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the live setLiveObject re-render is also mutating the SVG, so the layout is dirt…

🪰 Gadfly · advisory

🟠 **getBoundingClientRect() called on every pointermove during drags — forces layout reflow each move event (layout thrash)** _maintainability, performance · flagged by 4 models_ - **`web/src/editor/SelectionOverlay.tsx:60-65` — `getBoundingClientRect()` called on every `pointermove` during a drag.** `pointerWorld` reads `svgRef.current?.getBoundingClientRect()` and is invoked from the `trackPointer` move callback (line 78) on every pointer event. `getBoundingClientRect` forces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the live `setLiveObject` re-render is also mutating the SVG, so the layout is dirt… <sub>🪰 Gadfly · advisory</sub>
const pointerWorld = (e: PointerEvent): Point => { // The svg's screen rect doesn't move during a drag, so snapshot it once at
// gesture start rather than calling getBoundingClientRect (a layout reflow) on
// every pointermove.
const makePointerWorld = () => {
const rect = svgRef.current?.getBoundingClientRect() const rect = svgRef.current?.getBoundingClientRect()
return (e: { clientX: number; clientY: number }): Point => {
const vp = useEditorStore.getState().viewport const vp = useEditorStore.getState().viewport
const local = rect ? { x: e.clientX - rect.left, y: e.clientY - rect.top } : { x: e.clientX, y: e.clientY } const local = rect ? { x: e.clientX - rect.left, y: e.clientY - rect.top } : { x: e.clientX, y: e.clientY }
return screenToWorld(local, vp) return screenToWorld(local, vp)
} }
}
// Common gesture scaffolding: mark dragging, track the pointer, and on release // Common gesture scaffolding: mark dragging, track the pointer on window, and
// fire one PATCH built from the final liveObject. // on release fire one PATCH built from the final liveObject. onMove receives
// the live pointer event so modifier keys (Shift) reflect their current state.
const begin = ( const begin = (
e: ReactPointerEvent, e: ReactPointerEvent,
onMove: (world: Point) => EditorObject, onMove: (e: PointerEvent) => EditorObject,
fields: (final: EditorObject) => Parameters<typeof update.mutate>[0], fields: (final: EditorObject) => Parameters<typeof update.mutate>[0],
) => { ) => {
e.stopPropagation() e.stopPropagation()
e.preventDefault() e.preventDefault()
setObjectDragging(true) setObjectDragging(true)
trackPointer( const move = (ev: PointerEvent) => setLiveObject(onMove(ev))
(ev) => setLiveObject(onMove(pointerWorld(ev))), const detach = () => {
() => { window.removeEventListener('pointermove', move)
window.removeEventListener('pointerup', finish)
window.removeEventListener('pointercancel', finish)
}
const finish = () => {
detach()
Outdated
Review

🔴 Stale version captured in gesture closure causes spurious 409s

correctness · flagged by 1 model

  • web/src/editor/SelectionOverlay.tsx:88base = { ...object } snapshots the object's version at render time. The gesture-end PATCH sends base.version, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict during the drag. The fix is to read the current cached row's version when building the patch on release, or to derive it from t…

🪰 Gadfly · advisory

🔴 **Stale version captured in gesture closure causes spurious 409s** _correctness · flagged by 1 model_ - `web/src/editor/SelectionOverlay.tsx:88` — `base = { ...object }` snapshots the object's `version` at render time. The gesture-end PATCH sends `base.version`, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict *during* the drag. The fix is to read the current cached row's `version` when building the patch on release, or to derive it from t… <sub>🪰 Gadfly · advisory</sub>
cleanupRef.current = null
const final = useEditorStore.getState().liveObject const final = useEditorStore.getState().liveObject
setObjectDragging(false) setObjectDragging(false)
setLiveObject(null) setLiveObject(null)
if (final) update.mutate(fields(final)) if (final) update.mutate(fields(final))
}, }
) // On unmount mid-gesture: detach and reset, but don't fire a PATCH.
cleanupRef.current = () => {
detach()
setObjectDragging(false)
setLiveObject(null)
}
window.addEventListener('pointermove', move)
window.addEventListener('pointerup', finish)
window.addEventListener('pointercancel', finish)
} }
const base = { ...object } const base = { ...object }
const center0: Point = { x: base.xCm, y: base.yCm } const center0: Point = { x: base.xCm, y: base.yCm }
Review

🟡 Resize clamps size to MIN_OBJ_CM but keeps recomputing/moving the center when the dragged corner crosses the fixed opposite corner.

error-handling · flagged by 1 model

  • web/src/editor/SelectionOverlay.tsx:108 — resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner. newW/newH are clamped to MIN_OBJ_CM, but newCenterLocal is still derived from the live pointer position p, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor.

🪰 Gadfly · advisory

🟡 **Resize clamps size to MIN_OBJ_CM but keeps recomputing/moving the center when the dragged corner crosses the fixed opposite corner.** _error-handling · flagged by 1 model_ - **`web/src/editor/SelectionOverlay.tsx:108` — resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner.** `newW`/`newH` are clamped to `MIN_OBJ_CM`, but `newCenterLocal` is still derived from the live pointer position `p`, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor. <sub>🪰 Gadfly · advisory</sub>
const startMove = (e: ReactPointerEvent) => { const startMove = (e: ReactPointerEvent) => {
const pointerWorld = makePointerWorld()
const start = pointerWorld(e.nativeEvent) const start = pointerWorld(e.nativeEvent)
begin( begin(
e, e,
(world) => ({ ...base, xCm: base.xCm + (world.x - start.x), yCm: base.yCm + (world.y - start.y) }), (ev) => {
const world = pointerWorld(ev)
return { ...base, xCm: base.xCm + (world.x - start.x), yCm: base.yCm + (world.y - start.y) }
},
(f) => ({ id: base.id, version: base.version, xCm: f.xCm, yCm: f.yCm }), (f) => ({ id: base.id, version: base.version, xCm: f.xCm, yCm: f.yCm }),
) )
} }
1
@@ -101,9 +123,11 @@ export function SelectionOverlay({
// The corner opposite the dragged one stays fixed (in the object's local // The corner opposite the dragged one stays fixed (in the object's local
// frame, which is anchored at the original center). // frame, which is anchored at the original center).
const oppositeLocal: Point = { x: -sx * halfW, y: -sy * halfH } const oppositeLocal: Point = { x: -sx * halfW, y: -sy * halfH }
const pointerWorld = makePointerWorld()
begin( begin(
e, e,
Review

🟠 Shift key state locked at gesture start instead of evaluated live during rotate

correctness · flagged by 3 models

  • web/src/editor/SelectionOverlay.tsx:128 — Shift state captured at gesture start, not tracked during the drag. The onMove closure in startRotate references e.shiftKey from the original pointerdown event (if (!e.shiftKey) deg = Math.round(...)). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (ev.shiftKey) inside onMove instead of from e.

🪰 Gadfly · advisory

🟠 **Shift key state locked at gesture start instead of evaluated live during rotate** _correctness · flagged by 3 models_ - **`web/src/editor/SelectionOverlay.tsx:128` — Shift state captured at gesture start, not tracked during the drag.** The `onMove` closure in `startRotate` references `e.shiftKey` from the original `pointerdown` event (`if (!e.shiftKey) deg = Math.round(...)`). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (`ev.shiftKey`) inside `onMove` instead of from `e`. <sub>🪰 Gadfly · advisory</sub>
(world) => { (ev) => {
const world = pointerWorld(ev)
const p = worldToLocal(world, center0, base.rotationDeg) const p = worldToLocal(world, center0, base.rotationDeg)
const newW = Math.max(MIN_OBJ_CM, Math.abs(p.x - oppositeLocal.x)) const newW = Math.max(MIN_OBJ_CM, Math.abs(p.x - oppositeLocal.x))
const newH = Math.max(MIN_OBJ_CM, Math.abs(p.y - oppositeLocal.y)) const newH = Math.max(MIN_OBJ_CM, Math.abs(p.y - oppositeLocal.y))
1
@@ -120,12 +144,16 @@ export function SelectionOverlay({
} }
const startRotate = (e: ReactPointerEvent) => { const startRotate = (e: ReactPointerEvent) => {
const pointerWorld = makePointerWorld()
begin( begin(
e, e,
(world) => { (ev) => {
const world = pointerWorld(ev)
// +90° because the knob points up (local -y) at rotation 0. // +90° because the knob points up (local -y) at rotation 0.
let deg = (Math.atan2(world.y - center0.y, world.x - center0.x) * 180) / Math.PI + 90 let deg = (Math.atan2(world.y - center0.y, world.x - center0.x) * 180) / Math.PI + 90
if (!e.shiftKey) deg = Math.round(deg / ROTATE_SNAP_DEG) * ROTATE_SNAP_DEG // Read Shift live off the move event so toggling mid-drag switches
// between snapped and free rotation.
if (!ev.shiftKey) deg = Math.round(deg / ROTATE_SNAP_DEG) * ROTATE_SNAP_DEG
deg = ((deg % 360) + 360) % 360 deg = ((deg % 360) + 360) % 360
return { ...base, rotationDeg: deg } return { ...base, rotationDeg: deg }
}, },
+10 -7
View File
@@ -9,16 +9,19 @@ export interface KindDef {
shape: ObjectShapeKind shape: ObjectShapeKind
widthCm: number widthCm: number
heightCm: number heightCm: number
// Default z-index at placement. Paths/structures/in-ground sit under beds (0);
// beds and containers sit at 1; trees float above (2). Explicit z still wins.
defaultZ: number
} }
export const OBJECT_KINDS: KindDef[] = [ export const OBJECT_KINDS: KindDef[] = [
{ kind: 'bed', label: 'Bed', icon: '▭', shape: 'rect', widthCm: 120, heightCm: 240 }, { kind: 'bed', label: 'Bed', icon: '▭', shape: 'rect', widthCm: 120, heightCm: 240, defaultZ: 1 },
{ kind: 'grow_bag', label: 'Grow bag', icon: '🛍️', shape: 'circle', widthCm: 40, heightCm: 40 }, { kind: 'grow_bag', label: 'Grow bag', icon: '🛍️', shape: 'circle', widthCm: 40, heightCm: 40, defaultZ: 1 },
{ kind: 'container', label: 'Container', icon: '🪴', shape: 'circle', widthCm: 60, heightCm: 60 }, { kind: 'container', label: 'Container', icon: '🪴', shape: 'circle', widthCm: 60, heightCm: 60, defaultZ: 1 },
{ kind: 'in_ground', label: 'In-ground', icon: '🟫', shape: 'rect', widthCm: 200, heightCm: 200 }, { kind: 'in_ground', label: 'In-ground', icon: '🟫', shape: 'rect', widthCm: 200, heightCm: 200, defaultZ: 0 },
{ kind: 'tree', label: 'Tree', icon: '🌳', shape: 'circle', widthCm: 300, heightCm: 300 }, { kind: 'tree', label: 'Tree', icon: '🌳', shape: 'circle', widthCm: 300, heightCm: 300, defaultZ: 2 },
{ kind: 'path', label: 'Path', icon: '🧱', shape: 'rect', widthCm: 100, heightCm: 300 }, { kind: 'path', label: 'Path', icon: '🧱', shape: 'rect', widthCm: 100, heightCm: 300, defaultZ: 0 },
{ kind: 'structure', label: 'Structure', icon: '🏠', shape: 'rect', widthCm: 200, heightCm: 200 }, { kind: 'structure', label: 'Structure', icon: '🏠', shape: 'rect', widthCm: 200, heightCm: 200, defaultZ: 0 },
] ]
export function kindDef(kind: string): KindDef | undefined { export function kindDef(kind: string): KindDef | undefined {
+9 -2
View File
1
@@ -156,7 +156,10 @@ export function useUpdateObject(gardenId: number) {
toast.error(objectErrorMessage(err, 'Could not save that change.')) toast.error(objectErrorMessage(err, 'Could not save that change.'))
} }
}, },
onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }), // No onSettled refetch: onSuccess already writes the authoritative server
Outdated
Review

🟠 Redundant full-garden refetch (invalidateQueries) after every object PATCH already synced via onMutate/onSuccess

performance · flagged by 1 model

  • web/src/lib/objects.ts:159 (and 177)useUpdateObject and useDeleteObject both add onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }) even though onSuccess/onMutate already write the authoritative result into the cache (patchFullCache at lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch of GET /gardens/:id/full after every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires o…

🪰 Gadfly · advisory

🟠 **Redundant full-garden refetch (invalidateQueries) after every object PATCH already synced via onMutate/onSuccess** _performance · flagged by 1 model_ - **`web/src/lib/objects.ts:159` (and `177`)** — `useUpdateObject` and `useDeleteObject` both add `onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })` even though `onSuccess`/`onMutate` already write the authoritative result into the cache (`patchFullCache` at lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch of `GET /gardens/:id/full` after every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires o… <sub>🪰 Gadfly · advisory</sub>
// row and onError reconciles. Invalidating here would fire a full-garden GET
// after every PATCH — a request storm during drags, and a refetch race that
// can clobber a fast follow-up edit.
}) })
} }
@@ -184,10 +187,14 @@ function patchFullCache(qc: QueryClient, gardenId: number, fn: (full: FullGarden
qc.setQueryData<FullGarden>(fullKey(gardenId), (full) => (full ? fn(full) : full)) qc.setQueryData<FullGarden>(fullKey(gardenId), (full) => (full ? fn(full) : full))
} }
Review

🔴 Optimistic PATCH doesn't bump cached version, causing false 409s that silently discard the user's own rapid sequential edits

error-handling, maintainability · flagged by 4 models

  • web/src/lib/objects.ts:188-201 (applyServerPatch) — Ten near-identical ...(patch.X !== undefined ? { X: patch.X } : {}) lines, one per ObjectPatch field. Every time a field is added to ObjectPatch (objects.ts:108-121), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper.

🪰 Gadfly · advisory

🔴 **Optimistic PATCH doesn't bump cached version, causing false 409s that silently discard the user's own rapid sequential edits** _error-handling, maintainability · flagged by 4 models_ - `web/src/lib/objects.ts:188-201` (`applyServerPatch`) — Ten near-identical `...(patch.X !== undefined ? { X: patch.X } : {})` lines, one per `ObjectPatch` field. Every time a field is added to `ObjectPatch` (`objects.ts:108-121`), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper. <sub>🪰 Gadfly · advisory</sub>
/** Apply a patch onto a cached server object for the optimistic pass. */ /** Apply a patch onto a cached server object for the optimistic pass. The
* version is bumped to mirror the server's post-commit increment, so a second
* edit started before the first PATCH resolves reads the next version instead
* of re-sending the stale one and self-conflicting (a spurious 409). */
function applyServerPatch(o: ServerObject, patch: ObjectPatch): ServerObject { function applyServerPatch(o: ServerObject, patch: ObjectPatch): ServerObject {
return { return {
...o, ...o,
version: o.version + 1,
...(patch.name !== undefined ? { name: patch.name } : {}), ...(patch.name !== undefined ? { name: patch.name } : {}),
...(patch.xCm !== undefined ? { xCm: patch.xCm } : {}), ...(patch.xCm !== undefined ? { xCm: patch.xCm } : {}),
...(patch.yCm !== undefined ? { yCm: patch.yCm } : {}), ...(patch.yCm !== undefined ? { yCm: patch.yCm } : {}),
+7 -2
View File
@@ -1,4 +1,4 @@
import { useEffect } from 'react' import { useEffect, useMemo } from 'react'
import { getRouteApi } from '@tanstack/react-router' import { getRouteApi } from '@tanstack/react-router'
import { Alert } from '@/components/ui/Alert' import { Alert } from '@/components/ui/Alert'
import { GardenCanvas } from '@/editor/GardenCanvas' import { GardenCanvas } from '@/editor/GardenCanvas'
@@ -21,6 +21,12 @@ export function GardenEditorPage() {
const setArmedKind = useEditorStore((s) => s.setArmedKind) const setArmedKind = useEditorStore((s) => s.setArmedKind)
const setLiveObject = useEditorStore((s) => s.setLiveObject) const setLiveObject = useEditorStore((s) => s.setLiveObject)
// Map the server rows to EditorObjects once per cache change. Rebuilding this
// every render (e.g. on each viewport/selection update) would hand ObjectShape
// fresh identities and defeat its memo, re-rendering every object.
const serverObjects = full.data?.objects
const objects = useMemo(() => serverObjects?.map(toEditorObject) ?? [], [serverObjects])
// Clear transient editor state on entering/switching gardens and on leaving. // Clear transient editor state on entering/switching gardens and on leaving.
useEffect(() => { useEffect(() => {
const reset = () => { const reset = () => {
1
@@ -48,7 +54,6 @@ export function GardenEditorPage() {
heightCm: g.heightCm, heightCm: g.heightCm,
unitPref: g.unitPref, unitPref: g.unitPref,
} }
const objects = full.data.objects.map(toEditorObject)
const selected = objects.find((o) => o.id === selectedId) ?? null const selected = objects.find((o) => o.id === selectedId) ?? null
return ( return (