Polish: imperial, clear-bed, keyboard nudging, empty states (#18) #37

Merged
steve merged 2 commits from phase-7-polish into main 2026-07-19 04:44:26 +00:00
14 changed files with 268 additions and 11 deletions
+1
View File
@@ -4,6 +4,7 @@
<meta charset="UTF-8" /> <meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0, viewport-fit=cover" /> <meta name="viewport" content="width=device-width, initial-scale=1.0, viewport-fit=cover" />
<meta name="color-scheme" content="light dark" /> <meta name="color-scheme" content="light dark" />
<link rel="icon" href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 100 100'><text y='.9em' font-size='90'>%F0%9F%8C%B1</text></svg>" />
<title>pansy</title> <title>pansy</title>
<meta name="description" content="Self-hostable garden planner — plan beds, containers, and plops of plants at real scale." /> <meta name="description" content="Self-hostable garden planner — plan beds, containers, and plops of plants at real scale." />
</head> </head>
+20
View File
@@ -0,0 +1,20 @@
import { Link } from '@tanstack/react-router'
import { buttonClasses } from '@/components/ui/Button'
import { usePageTitle } from '@/lib/usePageTitle'
/** The router's catch-all for unknown paths. */
export function NotFound() {
usePageTitle('Not found')
return (
<div className="mx-auto flex min-h-[60vh] w-full max-w-sm flex-col items-center justify-center gap-4 text-center">
<p className="text-5xl" aria-hidden>
🌱
</p>
<h1 className="text-lg font-semibold text-fg">Page not found</h1>
<p className="text-sm text-muted">That page doesn't exist — the link may be wrong or the page moved.</p>
<Link to="/gardens" className={buttonClasses('primary')}>
Review

🟡 404 recovery link has no navigation-fallback error handling

error-handling · flagged by 1 model

  • buttonClasses('primary') on <Link> discards navigation semanticsweb/src/components/NotFound.tsx:15 Wrapping a TanStack Router <Link> with buttonClasses applies the visual style, but if buttonClasses ever gains type="button" or other button-specific attributes (it currently does not, but it is styled as a button), the component could inadvertently suppress default link behavior. More relevant to this lens: if the Link fails to resolve (e.g., router mismatch), there is no fa…

🪰 Gadfly · advisory

🟡 **404 recovery link has no navigation-fallback error handling** _error-handling · flagged by 1 model_ - **`buttonClasses('primary')` on `<Link>` discards navigation semantics** — `web/src/components/NotFound.tsx:15` Wrapping a TanStack Router `<Link>` with `buttonClasses` applies the visual style, but if `buttonClasses` ever gains `type="button"` or other button-specific attributes (it currently does not, but it is styled as a button), the component could inadvertently suppress default link behavior. More relevant to this lens: if the Link fails to resolve (e.g., router mismatch), there is no fa… <sub>🪰 Gadfly · advisory</sub>
Back to gardens
</Link>
</div>
)
}
+43
View File
@@ -0,0 +1,43 @@
import { Modal } from '@/components/ui/Modal'
import { Button } from '@/components/ui/Button'
import { useClearObject } from '@/lib/objects'
/** Confirm clearing every active plop from a focused bed (soft-remove — the rows
* are kept with removed_at, so history survives). */
export function ClearBedModal({
objectName,
plops,
gardenId,
onClose,
}: {
objectName: string
plops: { id: number; version: number }[]
gardenId: number
onClose: () => void
}) {
const clear = useClearObject(gardenId)
const n = plops.length
return (
<Modal title="Clear bed" onClose={onClose} busy={clear.isPending}>
<div className="flex flex-col gap-4">
<p className="text-sm text-muted">
Remove all <span className="font-medium text-fg">{n}</span> {n === 1 ? 'plant' : 'plants'} from{' '}
<span className="font-medium text-fg">{objectName}</span>? They're marked removed but kept in history.
</p>
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={clear.isPending}>
Cancel
</Button>
<Button
type="button"
variant="danger"
disabled={clear.isPending || n === 0}
onClick={() => clear.mutate(plops, { onSuccess: onClose })}
Review

🟠 Clear-bed button not disabled for empty plops array, causing no-op confirm

error-handling · flagged by 1 model

  • Unchecked empty-array guard in ClearBedModalweb/src/editor/ClearBedModal.tsx:35 If plops is empty (possible if the caller races with a concurrent delete or a stale focused-object ID), clear.mutate([]) fires a no-op mutation that succeeds instantly and closes the modal. It does not crash, but it silently consumes a user confirmation for nothing. More critically, useClearObject loops over the array with Promise.all; an empty array resolves immediately, making the UX misleading…

🪰 Gadfly · advisory

🟠 **Clear-bed button not disabled for empty plops array, causing no-op confirm** _error-handling · flagged by 1 model_ - **Unchecked empty-array guard in `ClearBedModal`** — `web/src/editor/ClearBedModal.tsx:35` If `plops` is empty (possible if the caller races with a concurrent delete or a stale focused-object ID), `clear.mutate([])` fires a no-op mutation that succeeds instantly and closes the modal. It does not crash, but it silently consumes a user confirmation for nothing. More critically, `useClearObject` loops over the array with `Promise.all`; an empty array resolves immediately, making the UX misleading… <sub>🪰 Gadfly · advisory</sub>
>
{clear.isPending ? 'Clearing' : 'Clear bed'}
</Button>
</div>
</div>
</Modal>
)
}
+18
View File
@@ -0,0 +1,18 @@
import type { ReactNode } from 'react'
import { cn } from '@/lib/cn'
/** A non-interactive hint overlaid on the canvas (empty-state guidance). */
export function EditorHint({ position = 'center', children }: { position?: 'center' | 'top'; children: ReactNode }) {
return (
<div
className={cn(
'pointer-events-none absolute flex p-6 text-center',
position === 'top' ? 'inset-x-0 top-16 justify-center' : 'inset-0 items-center justify-center',
)}
>
<p className="max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur">
{children}
</p>
</div>
)
}
+4
View File
@@ -37,6 +37,9 @@ export const PlopMarker = memo(function PlopMarker({
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) const count = plop.count ?? (plant ? computeDerivedCount(plop.radiusCm, plant.spacingCm) : plop.derivedCount)
// Keep the tap target at least ~44px across even when the plop draws tiny at
// low zoom (fill=transparent still receives pointer events, unlike fill=none).
const hitR = Math.max(r, 22 / scale)
function down(e: PointerEvent) { function down(e: PointerEvent) {
e.stopPropagation() e.stopPropagation()
@@ -45,6 +48,7 @@ export const PlopMarker = memo(function PlopMarker({
return ( return (
<g transform={`translate(${plop.xCm} ${plop.yCm})`} onPointerDown={down} style={{ cursor: 'pointer' }}> <g transform={`translate(${plop.xCm} ${plop.yCm})`} onPointerDown={down} style={{ cursor: 'pointer' }}>
<circle cx={0} cy={0} r={hitR} fill="transparent" />
<circle <circle
cx={0} cx={0}
cy={0} cy={0}
+5
View File
@@ -27,3 +27,8 @@ export const OBJECT_KINDS: KindDef[] = [
export function kindDef(kind: string): KindDef | undefined { export function kindDef(kind: string): KindDef | undefined {
return OBJECT_KINDS.find((k) => k.kind === kind) return OBJECT_KINDS.find((k) => k.kind === kind)
} }
/** A human label for an object: its name, else its kind's label, else "Object". */
export function objectDisplayName(o: { name: string; kind: string }): string {
return o.name || kindDef(o.kind)?.label || 'Object'
}
+25
View File
@@ -263,6 +263,31 @@ export function useUpdatePlanting(gardenId: number) {
}) })
} }
/** Clear a bed: soft-remove every active plop in an object (a loop of PATCHes;
* a bulk ClearObject endpoint arrives with the agent seam, #19). Invalidates
* once at the end. Pass the object's active plops (id + current version). */
export function useClearObject(gardenId: number) {
Review

🔴 useClearObject lacks optimistic update and partial-failure handling

correctness, error-handling, maintainability · flagged by 4 models

  • web/src/lib/objects.ts:269-281useClearObject has no optimistic update and no partial-failure handling Promise.all patches each plop individually. If one request fails, onError fires but onSuccess does not, so the cache is never invalidated. Any plops that were successfully soft-removed on the server still appear in the UI until a manual refresh. There is also no 409 conflict recovery (unlike useRemovePlanting). Fix: Add an onMutate that filters the plops from the cache…

🪰 Gadfly · advisory

🔴 **useClearObject lacks optimistic update and partial-failure handling** _correctness, error-handling, maintainability · flagged by 4 models_ - **`web/src/lib/objects.ts:269-281` — `useClearObject` has no optimistic update and no partial-failure handling** `Promise.all` patches each plop individually. If one request fails, `onError` fires but `onSuccess` does not, so the cache is never invalidated. Any plops that were successfully soft-removed on the server still appear in the UI until a manual refresh. There is also no 409 conflict recovery (unlike `useRemovePlanting`). **Fix:** Add an `onMutate` that filters the plops from the cache… <sub>🪰 Gadfly · advisory</sub>
const qc = useQueryClient()
return useMutation({
mutationFn: async (plops: { id: number; version: number }[]) => {
const today = new Date().toISOString().slice(0, 10)
// allSettled, not all: a partial failure still soft-removed some rows
// server-side, so we must reconcile the cache rather than roll everything
// back. Report how many failed.
const results = await Promise.allSettled(
plops.map((p) => api.patch(`/plantings/${p.id}`, { removedAt: today, version: p.version })),
Review

🟠 useClearObject: Promise.all rejection on one PATCH leaves other successful soft-removes uncommitted to the cache — no invalidate/reconciliation on error, so UI misreports partial failures as total failure

correctness, error-handling · flagged by 2 models

  • web/src/lib/objects.ts:278useClearObject fires a Promise.all of soft-remove PATCHes but has no optimistic onMutate and only invalidates on onSuccess. If one PATCH fails (e.g., version conflict) while others succeed, onError fires, the cache is never invalidated, and the UI continues displaying plants that were already removed on the server. A retry will then 409 on the already-removed rows because their versions were bumped by the successful PATCHes. Fix: add an `onMutat…

🪰 Gadfly · advisory

🟠 **useClearObject: Promise.all rejection on one PATCH leaves other successful soft-removes uncommitted to the cache — no invalidate/reconciliation on error, so UI misreports partial failures as total failure** _correctness, error-handling · flagged by 2 models_ * `web/src/lib/objects.ts:278` — `useClearObject` fires a `Promise.all` of soft-remove PATCHes but has **no optimistic `onMutate`** and only invalidates on `onSuccess`. If one PATCH fails (e.g., version conflict) while others succeed, `onError` fires, the cache is never invalidated, and the UI continues displaying plants that were already removed on the server. A retry will then 409 on the already-removed rows because their versions were bumped by the successful PATCHes. **Fix:** add an `onMutat… <sub>🪰 Gadfly · advisory</sub>
)
const failed = results.filter((r) => r.status === 'rejected').length
if (failed > 0) {
throw new Error(`${failed} of ${plops.length} plants couldn't be cleared — refresh and try again.`)
}
},
// Reconcile on success OR partial failure, so the cache matches the server.
onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),
onError: (err) => toast.error(err instanceof Error ? err.message : 'Could not clear the bed.'),
})
}
/** Soft-remove ("Remove"): PATCH removed_at to today; the row is kept but leaves /** Soft-remove ("Remove"): PATCH removed_at to today; the row is kept but leaves
* the active list. Distinct from a hard delete. */ * the active list. Distinct from a hard delete. */
export function useRemovePlanting(gardenId: number) { export function useRemovePlanting(gardenId: number) {
+10
View File
@@ -0,0 +1,10 @@
import { useEffect } from 'react'
/** Set the document title for the current route (suffixed with · pansy). No
* restore-on-unmount: each route sets its own title, so restoring the previous
* one would race and clobber the incoming route's title on a fast navigation. */
export function usePageTitle(title: string): void {
Review

🟠 Page title restoration races on rapid route changes, can clobber active title

correctness, error-handling, maintainability · flagged by 3 models

  • web/src/lib/usePageTitle.ts:6-12usePageTitle restore-on-unmount can restore a stale route title across SPA navigations (minor). On mount it captures prev = document.title (the previous route's title) and restores that exact string on unmount. Navigating Gardens → GardenEditor → Plants: the GardenEditor effect captures prev = "Gardens · pansy"; when GardenEditor unmounts it restores "Gardens · pansy" instead of a neutral default, and depending on React's effect/cleanup orderi…

🪰 Gadfly · advisory

🟠 **Page title restoration races on rapid route changes, can clobber active title** _correctness, error-handling, maintainability · flagged by 3 models_ - **`web/src/lib/usePageTitle.ts:6-12` — `usePageTitle` restore-on-unmount can restore a stale route title across SPA navigations (minor).** On mount it captures `prev = document.title` (the *previous* route's title) and restores that exact string on unmount. Navigating Gardens → GardenEditor → Plants: the GardenEditor effect captures `prev = "Gardens · pansy"`; when GardenEditor unmounts it restores `"Gardens · pansy"` instead of a neutral default, and depending on React's effect/cleanup orderi… <sub>🪰 Gadfly · advisory</sub>
useEffect(() => {
document.title = title ? `${title} · pansy` : 'pansy'
}, [title])
}
+131 -11
View File
@@ -1,4 +1,4 @@
import { useEffect, useMemo, useState } from 'react' import { useEffect, useMemo, useRef, useState } 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 { Button } from '@/components/ui/Button' import { Button } from '@/components/ui/Button'
@@ -7,13 +7,16 @@ import { Inspector } from '@/editor/Inspector'
import { PlopInspector } from '@/editor/PlopInspector' import { PlopInspector } from '@/editor/PlopInspector'
import { PlantPicker } from '@/editor/PlantPicker' import { PlantPicker } from '@/editor/PlantPicker'
import { Palette } from '@/editor/Palette' import { Palette } from '@/editor/Palette'
import { kindDef } from '@/editor/kinds' import { ClearBedModal } from '@/editor/ClearBedModal'
import { EditorHint } from '@/editor/EditorHint'
import { objectDisplayName } from '@/editor/kinds'
import { useEditorStore } from '@/editor/store' import { useEditorStore } from '@/editor/store'
import type { EditorGarden } from '@/editor/types' import type { EditorGarden } from '@/editor/types'
import { ShareGardenModal } from '@/components/gardens/ShareGardenModal' import { ShareGardenModal } from '@/components/gardens/ShareGardenModal'
import { useMe } from '@/lib/auth' import { useMe } from '@/lib/auth'
import { toEditorObject, useGardenFull, useUpdatePlanting } from '@/lib/objects' import { toEditorObject, useGardenFull, useUpdateObject, useUpdatePlanting } from '@/lib/objects'
import { toEditorPlanting } from '@/lib/plantings' import { toEditorPlanting } from '@/lib/plantings'
import { usePageTitle } from '@/lib/usePageTitle'
const routeApi = getRouteApi('/gardens/$gardenId') const routeApi = getRouteApi('/gardens/$gardenId')
@@ -24,6 +27,7 @@ export function GardenEditorPage() {
const navigate = routeApi.useNavigate() const navigate = routeApi.useNavigate()
const full = useGardenFull(gid) const full = useGardenFull(gid)
const me = useMe() const me = useMe()
usePageTitle(full.data?.garden.name ?? 'Garden')
const selectedId = useEditorStore((s) => s.selectedId) const selectedId = useEditorStore((s) => s.selectedId)
const select = useEditorStore((s) => s.select) const select = useEditorStore((s) => s.select)
@@ -38,11 +42,15 @@ export function GardenEditorPage() {
const setLivePlanting = useEditorStore((s) => s.setLivePlanting) const setLivePlanting = useEditorStore((s) => s.setLivePlanting)
const updatePlanting = useUpdatePlanting(gid) const updatePlanting = useUpdatePlanting(gid)
const updateObject = useUpdateObject(gid)
// Which plant-picker flow is open: 'place' arms a plant for repeat placement; // Which plant-picker flow is open: 'place' arms a plant for repeat placement;
// 'change' swaps the selected plop's plant. // 'change' swaps the selected plop's plant.
const [picker, setPicker] = useState<'place' | 'change' | null>(null) const [picker, setPicker] = useState<'place' | 'change' | null>(null)
const [sharing, setSharing] = useState(false) const [sharing, setSharing] = useState(false)
const [clearing, setClearing] = useState(false)
const nudgeTimer = useRef<number | null>(null)
const nudgeFire = useRef<(() => void) | null>(null)
const serverObjects = full.data?.objects const serverObjects = full.data?.objects
const objects = useMemo(() => serverObjects?.map(toEditorObject) ?? [], [serverObjects]) const objects = useMemo(() => serverObjects?.map(toEditorObject) ?? [], [serverObjects])
@@ -51,6 +59,17 @@ export function GardenEditorPage() {
const plants = useMemo(() => full.data?.plants ?? [], [full.data?.plants]) const plants = useMemo(() => full.data?.plants ?? [], [full.data?.plants])
const plantsById = useMemo(() => new Map(plants.map((p) => [p.id, p])), [plants]) const plantsById = useMemo(() => new Map(plants.map((p) => [p.id, p])), [plants])
// Role gating, computed before the effects/returns so the nudge handler can use
// it. Ownership is the authoritative ownerId==me check.
Review

canEdit and isOwner use inconsistent null-check idioms for the same ownerId check

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **canEdit and isOwner use inconsistent null-check idioms for the same ownerId check** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
const gd = full.data?.garden
const canEdit = gd != null && (gd.ownerId === me.data?.id || gd.myRole === 'editor')
const isOwner = gd != null && me.data != null && gd.ownerId === me.data.id
// Latest values for the mount-once nudge keydown handler to read, so its effect
// never re-subscribes (which would cancel a pending debounced commit).
const nudgeCtx = useRef({ canEdit, objects, plantings, updateObject, updatePlanting })
nudgeCtx.current = { canEdit, objects, plantings, updateObject, updatePlanting }
// Adopt the URL's focus and clear transient editor state on entering/switching // Adopt the URL's focus and clear transient editor state on entering/switching
// gardens (and on leaving). `focus` is intentionally read once here, not a dep, // gardens (and on leaving). `focus` is intentionally read once here, not a dep,
// so URL syncs below don't retrigger a full reset. // so URL syncs below don't retrigger a full reset.
@@ -108,6 +127,87 @@ export function GardenEditorPage() {
// eslint-disable-next-line react-hooks/exhaustive-deps // eslint-disable-next-line react-hooks/exhaustive-deps
}, []) }, [])
// Desktop keyboard nudging: arrows move the selected object/plop 1cm (Shift =
// 10cm); the PATCH is debounced ~400ms on key-idle so a held key doesn't spam.
// Plops nudge in their object's local frame and clamp to its (local) bounds.
// Mounted once, reading live values from nudgeCtx so a data refetch can't
// re-subscribe and cancel a pending commit; the pending commit is flushed on
// unmount, and a fire only commits if its live value is still present (a drag
// that cleared it already committed its own PATCH).
useEffect(() => {
const DIRS: Record<string, [number, number]> = {
ArrowUp: [0, -1],
ArrowDown: [0, 1],
ArrowLeft: [-1, 0],
ArrowRight: [1, 0],
}
const commitLater = (fire: () => void) => {
nudgeFire.current = fire
if (nudgeTimer.current != null) window.clearTimeout(nudgeTimer.current)
nudgeTimer.current = window.setTimeout(() => {
nudgeTimer.current = null
Review

🟡 Object-nudge and plop-nudge branches duplicate the same find-base/compute-next/setLive/commitLater structure

maintainability · flagged by 3 models

  • web/src/pages/GardenEditorPage.tsx:148-178 — the keyboard-nudge onKey handler has two branches (object nudge vs plop nudge) that repeat the same shape: resolve base from live-state-or-list, e.preventDefault(), compute next, call the matching setLive*, then commitLater(() => { mutate(...); resetLive() }). Confirmed by reading the full effect (lines 126-186) — the only real differences are the source list/live-state pair, the optional bounds clamp for plops, and which mutation hook…

🪰 Gadfly · advisory

🟡 **Object-nudge and plop-nudge branches duplicate the same find-base/compute-next/setLive/commitLater structure** _maintainability · flagged by 3 models_ - `web/src/pages/GardenEditorPage.tsx:148-178` — the keyboard-nudge `onKey` handler has two branches (object nudge vs plop nudge) that repeat the same shape: resolve `base` from live-state-or-list, `e.preventDefault()`, compute `next`, call the matching `setLive*`, then `commitLater(() => { mutate(...); resetLive() })`. Confirmed by reading the full effect (lines 126-186) — the only real differences are the source list/live-state pair, the optional bounds clamp for plops, and which mutation hook… <sub>🪰 Gadfly · advisory</sub>
const fn = nudgeFire.current
nudgeFire.current = null
fn?.()
}, 400)
}
function onKey(e: KeyboardEvent) {
Review

🔴 Nudge debounce timer can stomp newer liveObject/livePlanting from a subsequent drag gesture

error-handling · flagged by 1 model

  • web/src/pages/GardenEditorPage.tsx:154 — Debounced nudge timer can clear a newer live gesture The commitLater closure calls setLiveObject(null) / setLivePlanting(null) when it fires, without checking whether liveObject/livePlanting is still the nudged entity. If the user starts dragging another object (or another plop) during the 400 ms debounce window, the timer will overwrite that newer live state and snap the dragged item back to its committed position. Fix: In the timer…

🪰 Gadfly · advisory

🔴 **Nudge debounce timer can stomp newer liveObject/livePlanting from a subsequent drag gesture** _error-handling · flagged by 1 model_ - **`web/src/pages/GardenEditorPage.tsx:154` — Debounced nudge timer can clear a newer live gesture** The `commitLater` closure calls `setLiveObject(null)` / `setLivePlanting(null)` when it fires, without checking whether `liveObject`/`livePlanting` is still the nudged entity. If the user starts dragging another object (or another plop) during the 400 ms debounce window, the timer will overwrite that newer live state and snap the dragged item back to its committed position. **Fix:** In the timer… <sub>🪰 Gadfly · advisory</sub>
const { canEdit: canNudge, objects: objs, plantings: plops, updateObject: uo, updatePlanting: up } =
nudgeCtx.current
if (!canNudge) return
const dir = DIRS[e.key]
if (!dir) return
const el = document.activeElement
if (el && (el.tagName === 'INPUT' || el.tagName === 'TEXTAREA' || el.tagName === 'SELECT')) return
const s = useEditorStore.getState()
if (s.objectDragging) return // don't fight an active pointer drag
const step = e.shiftKey ? 10 : 1
if (s.selectedId != null) {
const base = s.liveObject?.id === s.selectedId ? s.liveObject : objs.find((o) => o.id === s.selectedId)
if (!base) return
e.preventDefault()
Review

🔴 Nudge clamping ignores object rotation, letting plops escape rotated beds

error-handling · flagged by 1 model

  • Boundary / off-by-one in nudge clamping for non-square objectsweb/src/pages/GardenEditorPage.tsx:168-171 Plops are clamped to [-widthCm/2, widthCm/2] and [-heightCm/2, heightCm/2]. If the object has been rotated (rotationDeg != 0), the plop’s local xCm/yCm frame is rotated relative to the object’s bounding box; clamping axis-aligned coordinates against the unrotated width/height allows the plop to drift outside the actual bed. Fix: apply the inverse rotation to the plop co…

🪰 Gadfly · advisory

🔴 **Nudge clamping ignores object rotation, letting plops escape rotated beds** _error-handling · flagged by 1 model_ - **Boundary / off-by-one in nudge clamping for non-square objects** — `web/src/pages/GardenEditorPage.tsx:168-171` Plops are clamped to `[-widthCm/2, widthCm/2]` and `[-heightCm/2, heightCm/2]`. If the object has been rotated (`rotationDeg != 0`), the plop’s local `xCm/yCm` frame is rotated relative to the object’s bounding box; clamping axis-aligned coordinates against the unrotated width/height allows the plop to drift outside the actual bed. **Fix:** apply the inverse rotation to the plop co… <sub>🪰 Gadfly · advisory</sub>
s.setLiveObject({ ...base, xCm: base.xCm + dir[0] * step, yCm: base.yCm + dir[1] * step })
commitLater(() => {
const live = useEditorStore.getState().liveObject
if (live?.id !== s.selectedId) return // a drag cleared it and committed
uo.mutate({ id: live.id, version: live.version, xCm: live.xCm, yCm: live.yCm })
useEditorStore.getState().setLiveObject(null)
})
} else if (s.selectedPlantingId != null) {
const base =
s.livePlanting?.id === s.selectedPlantingId ? s.livePlanting : plops.find((p) => p.id === s.selectedPlantingId)
if (!base) return
e.preventDefault()
const obj = objs.find((o) => o.id === base.objectId)
let nx = base.xCm + dir[0] * step
let ny = base.yCm + dir[1] * step
Review

🔴 Debounced keyboard-nudge commit is discarded (not flushed) on effect cleanup, silently dropping unsaved moves while the canvas keeps showing the phantom position

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

  • web/src/pages/GardenEditorPage.tsx:186 — The keyboard nudge useEffect lists objects and plantings in its dependency array. Because patchFullCache (called by useUpdateObject / useUpdatePlanting optimistic updates) returns new array references on every cache mutation, this effect tears down and re-adds the global keydown listener repeatedly during normal editing. The previous effect’s cleanup clears nudgeTimer.current, so if any mutation patches the garden cache while a…

🪰 Gadfly · advisory

🔴 **Debounced keyboard-nudge commit is discarded (not flushed) on effect cleanup, silently dropping unsaved moves while the canvas keeps showing the phantom position** _correctness, error-handling, performance · flagged by 2 models_ - **`web/src/pages/GardenEditorPage.tsx:186`** — The keyboard nudge `useEffect` lists `objects` and `plantings` in its dependency array. Because `patchFullCache` (called by `useUpdateObject` / `useUpdatePlanting` optimistic updates) returns new array references on every cache mutation, this effect tears down and re-adds the global `keydown` listener repeatedly during normal editing. The previous effect’s cleanup clears `nudgeTimer.current`, so if **any** mutation patches the garden cache while a… <sub>🪰 Gadfly · advisory</sub>
if (obj) {
nx = Math.max(-obj.widthCm / 2, Math.min(obj.widthCm / 2, nx))
ny = Math.max(-obj.heightCm / 2, Math.min(obj.heightCm / 2, ny))
}
s.setLivePlanting({ ...base, xCm: nx, yCm: ny })
commitLater(() => {
const live = useEditorStore.getState().livePlanting
if (live?.id !== s.selectedPlantingId) return
up.mutate({ id: live.id, version: live.version, xCm: live.xCm, yCm: live.yCm })
useEditorStore.getState().setLivePlanting(null)
})
}
}
window.addEventListener('keydown', onKey)
return () => {
window.removeEventListener('keydown', onKey)
if (nudgeTimer.current != null) {
window.clearTimeout(nudgeTimer.current)
nudgeTimer.current = null
}
const fn = nudgeFire.current // flush a pending move so it isn't lost
nudgeFire.current = null
fn?.()
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [])
if (full.isPending) return <p className="p-6 text-sm text-muted">Loading garden</p> if (full.isPending) return <p className="p-6 text-sm text-muted">Loading garden</p>
if (full.isError) if (full.isError)
return ( return (
@@ -124,16 +224,12 @@ export function GardenEditorPage() {
heightCm: g.heightCm, heightCm: g.heightCm,
unitPref: g.unitPref, unitPref: g.unitPref,
} }
// Role-driven gating. Ownership is the authoritative ownerId==me check (so a
// missing my_role can never lock an owner out); edit access is owner or an
// editor share.
const isOwner = me.data != null && g.ownerId === me.data.id
const canEdit = isOwner || g.myRole === 'editor'
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
// Committed selected plop; PlopInspector applies live drag geometry itself. // Committed selected plop; PlopInspector applies live drag geometry itself.
const selectedPlop = plantings.find((p) => p.id === selectedPlantingId) ?? null const selectedPlop = plantings.find((p) => p.id === selectedPlantingId) ?? null
const focusedPlops = focusedObjectId != null ? plantings.filter((p) => p.objectId === focusedObjectId) : []
function onPickPlant(plantId: number) { function onPickPlant(plantId: number) {
if (!canEdit) return // defense in depth: viewers can't reach the picker anyway if (!canEdit) return // defense in depth: viewers can't reach the picker anyway
@@ -170,9 +266,7 @@ export function GardenEditorPage() {
<button type="button" onClick={exitFocus} className="rounded px-1.5 py-0.5 font-medium text-accent-strong hover:underline"> <button type="button" onClick={exitFocus} className="rounded px-1.5 py-0.5 font-medium text-accent-strong hover:underline">
{garden.name} {garden.name}
</button> </button>
<span className="max-w-[8rem] truncate text-muted"> <span className="max-w-[8rem] truncate text-muted">{objectDisplayName(focusedObject)}</span>
{focusedObject.name || kindDef(focusedObject.kind)?.label || 'Object'}
</span>
{canEdit && {canEdit &&
(!focusedObject.plantable ? ( (!focusedObject.plantable ? (
<span className="text-xs text-muted">Not plantable</span> <span className="text-xs text-muted">Not plantable</span>
@@ -185,9 +279,26 @@ export function GardenEditorPage() {
+ Add plant + Add plant
</Button> </Button>
))} ))}
{canEdit && focusedObject.plantable && focusedPlops.length > 0 && (
<Button
variant="ghost"
className="px-2 py-1 text-xs text-red-600 dark:text-red-400"
onClick={() => setClearing(true)}
>
Clear ({focusedPlops.length})
</Button>
)}
</div> </div>
)} )}
<GardenCanvas garden={garden} objects={objects} plantings={plantings} plantsById={plantsById} canEdit={canEdit} /> <GardenCanvas garden={garden} objects={objects} plantings={plantings} plantsById={plantsById} canEdit={canEdit} />
{/* Empty-state hints (non-interactive overlays). */}
{canEdit && focusedObjectId == null && objects.length === 0 && (
<EditorHint>Pick a shape from the palette, then tap the field to place your first bed.</EditorHint>
)}
{canEdit && focusedObject?.plantable && focusedPlops.length === 0 && !armedPlant && (
<EditorHint position="top">Tap + Add plant, then tap inside the bed to place plops.</EditorHint>
)}
</div> </div>
{(selectedObject || selectedPlop) && ( {(selectedObject || selectedPlop) && (
@@ -229,6 +340,15 @@ export function GardenEditorPage() {
)} )}
{sharing && <ShareGardenModal garden={g} onClose={() => setSharing(false)} />} {sharing && <ShareGardenModal garden={g} onClose={() => setSharing(false)} />}
{clearing && focusedObject && (
<ClearBedModal
objectName={objectDisplayName(focusedObject)}
plops={focusedPlops.map((p) => ({ id: p.id, version: p.version }))}
gardenId={gid}
onClose={() => setClearing(false)}
/>
)}
</div> </div>
) )
} }
+2
View File
@@ -8,6 +8,7 @@ import { LeaveGardenModal } from '@/components/gardens/LeaveGardenModal'
import { ShareGardenModal } from '@/components/gardens/ShareGardenModal' import { ShareGardenModal } from '@/components/gardens/ShareGardenModal'
import { useMe } from '@/lib/auth' import { useMe } from '@/lib/auth'
import { useGardens, type Garden } from '@/lib/gardens' import { useGardens, type Garden } from '@/lib/gardens'
import { usePageTitle } from '@/lib/usePageTitle'
// Which modal is open, if any. edit/delete/share/leave carry the target garden. // Which modal is open, if any. edit/delete/share/leave carry the target garden.
type Dialog = type Dialog =
@@ -19,6 +20,7 @@ type Dialog =
| null | null
export function GardensPage() { export function GardensPage() {
usePageTitle('Gardens')
const gardens = useGardens() const gardens = useGardens()
const me = useMe() const me = useMe()
const [dialog, setDialog] = useState<Dialog>(null) const [dialog, setDialog] = useState<Dialog>(null)
+2
View File
@@ -8,6 +8,7 @@ import { TextField } from '@/components/ui/TextField'
import { API_BASE, errorMessage } from '@/lib/api' import { API_BASE, errorMessage } from '@/lib/api'
import { useLogin, useProviders } from '@/lib/auth' import { useLogin, useProviders } from '@/lib/auth'
import { safeRedirectPath } from '@/lib/redirect' import { safeRedirectPath } from '@/lib/redirect'
import { usePageTitle } from '@/lib/usePageTitle'
// Server-initiated redirect (not a fetch): the browser navigates to the Go // Server-initiated redirect (not a fetch): the browser navigates to the Go
// handler, which 302s to the IdP. // handler, which 302s to the IdP.
@@ -24,6 +25,7 @@ const callbackErrors: Record<string, string> = {
} }
export function LoginPage() { export function LoginPage() {
usePageTitle('Sign in')
const search = useSearch({ from: '/login' }) const search = useSearch({ from: '/login' })
const navigate = useNavigate() const navigate = useNavigate()
const providers = useProviders() const providers = useProviders()
+2
View File
@@ -10,6 +10,7 @@ import { DeletePlantModal } from '@/components/plants/DeletePlantModal'
import { PlantPicker } from '@/editor/PlantPicker' import { PlantPicker } from '@/editor/PlantPicker'
import { filterPlants, usePlants, type CategoryFilter, type Plant } from '@/lib/plants' import { filterPlants, usePlants, type CategoryFilter, type Plant } from '@/lib/plants'
import type { UnitPref } from '@/lib/units' import type { UnitPref } from '@/lib/units'
import { usePageTitle } from '@/lib/usePageTitle'
// Which modal is open, if any. edit/duplicate/delete carry the target plant. // Which modal is open, if any. edit/duplicate/delete carry the target plant.
type Dialog = type Dialog =
@@ -32,6 +33,7 @@ function loadUnit(): UnitPref {
} }
export function PlantsPage() { export function PlantsPage() {
usePageTitle('Plants')
const plants = usePlants() const plants = usePlants()
const [unit, setUnit] = useState<UnitPref>(() => loadUnit()) const [unit, setUnit] = useState<UnitPref>(() => loadUnit())
const [query, setQuery] = useState('') const [query, setQuery] = useState('')
+2
View File
@@ -6,10 +6,12 @@ import { Button } from '@/components/ui/Button'
import { TextField } from '@/components/ui/TextField' import { TextField } from '@/components/ui/TextField'
import { apiErrorCode, errorMessage } from '@/lib/api' import { apiErrorCode, errorMessage } from '@/lib/api'
import { useProviders, useRegister } from '@/lib/auth' import { useProviders, useRegister } from '@/lib/auth'
import { usePageTitle } from '@/lib/usePageTitle'
const MIN_PASSWORD = 8 const MIN_PASSWORD = 8
export function RegisterPage() { export function RegisterPage() {
usePageTitle('Create account')
const navigate = useNavigate() const navigate = useNavigate()
const providers = useProviders() const providers = useProviders()
const register = useRegister() const register = useRegister()
+3
View File
@@ -6,6 +6,7 @@ import {
} from '@tanstack/react-router' } from '@tanstack/react-router'
import type { QueryClient } from '@tanstack/react-query' import type { QueryClient } from '@tanstack/react-query'
import { AppShell } from '@/components/layout/AppShell' import { AppShell } from '@/components/layout/AppShell'
import { NotFound } from '@/components/NotFound'
import { RouteError } from '@/components/RouteError' import { RouteError } from '@/components/RouteError'
import { LoginPage } from '@/pages/LoginPage' import { LoginPage } from '@/pages/LoginPage'
import { RegisterPage } from '@/pages/RegisterPage' import { RegisterPage } from '@/pages/RegisterPage'
@@ -25,6 +26,8 @@ const rootRoute = createRootRouteWithContext<RouterContext>()({
// A beforeLoad/loader failure that isn't a redirect (e.g. /auth/me errors with // A beforeLoad/loader failure that isn't a redirect (e.g. /auth/me errors with
// a 500 or the network drops) lands here instead of a blank screen. // a 500 or the network drops) lands here instead of a blank screen.
errorComponent: RouteError, errorComponent: RouteError,
// Unknown paths render inside the app shell rather than a blank screen.
notFoundComponent: NotFound,
}) })
// requireAuth: resolve the current user (shared cache with useMe); send anyone // requireAuth: resolve the current user (shared cache with useMe); send anyone