Polish: imperial, clear-bed, keyboard nudging, empty states (#18) #37
@@ -31,7 +31,7 @@ export function ClearBedModal({
|
||||
<Button
|
||||
type="button"
|
||||
variant="danger"
|
||||
disabled={clear.isPending}
|
||||
disabled={clear.isPending || n === 0}
|
||||
onClick={() => clear.mutate(plops, { onSuccess: onClose })}
|
||||
|
|
||||
>
|
||||
{clear.isPending ? 'Clearing…' : 'Clear bed'}
|
||||
|
||||
@@ -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>
|
||||
)
|
||||
}
|
||||
@@ -27,3 +27,8 @@ export const OBJECT_KINDS: KindDef[] = [
|
||||
export function kindDef(kind: string): KindDef | undefined {
|
||||
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'
|
||||
}
|
||||
|
||||
@@ -271,12 +271,20 @@ export function useClearObject(gardenId: number) {
|
||||
return useMutation({
|
||||
mutationFn: async (plops: { id: number; version: number }[]) => {
|
||||
const today = new Date().toISOString().slice(0, 10)
|
||||
await Promise.all(
|
||||
// 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 })),
|
||||
|
gitea-actions
commented
🟠 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
🪰 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.`)
|
||||
}
|
||||
},
|
||||
onSuccess: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),
|
||||
onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')),
|
||||
// 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.'),
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -1,13 +1,10 @@
|
||||
import { useEffect } from 'react'
|
||||
|
||||
/** Set the document title for the current route (suffixed with · pansy), and
|
||||
* restore the previous title on unmount. */
|
||||
/** 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 {
|
||||
|
gitea-actions
commented
🟠 Page title restoration races on rapid route changes, can clobber active title correctness, error-handling, maintainability · flagged by 3 models
🪰 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(() => {
|
||||
const prev = document.title
|
||||
document.title = title ? `${title} · pansy` : 'pansy'
|
||||
return () => {
|
||||
document.title = prev
|
||||
}
|
||||
}, [title])
|
||||
}
|
||||
|
||||
@@ -8,7 +8,8 @@ import { PlopInspector } from '@/editor/PlopInspector'
|
||||
import { PlantPicker } from '@/editor/PlantPicker'
|
||||
import { Palette } from '@/editor/Palette'
|
||||
import { ClearBedModal } from '@/editor/ClearBedModal'
|
||||
import { kindDef } from '@/editor/kinds'
|
||||
import { EditorHint } from '@/editor/EditorHint'
|
||||
import { objectDisplayName } from '@/editor/kinds'
|
||||
import { useEditorStore } from '@/editor/store'
|
||||
import type { EditorGarden } from '@/editor/types'
|
||||
import { ShareGardenModal } from '@/components/gardens/ShareGardenModal'
|
||||
@@ -49,6 +50,7 @@ export function GardenEditorPage() {
|
||||
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 objects = useMemo(() => serverObjects?.map(toEditorObject) ?? [], [serverObjects])
|
||||
@@ -63,6 +65,11 @@ export function GardenEditorPage() {
|
||||
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
|
||||
// gardens (and on leaving). `focus` is intentionally read once here, not a dep,
|
||||
// so URL syncs below don't retrigger a full reset.
|
||||
@@ -122,9 +129,12 @@ export function GardenEditorPage() {
|
||||
|
||||
// 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 bounds.
|
||||
// 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(() => {
|
||||
if (!canEdit) return
|
||||
const DIRS: Record<string, [number, number]> = {
|
||||
ArrowUp: [0, -1],
|
||||
ArrowDown: [0, 1],
|
||||
@@ -132,47 +142,54 @@ export function GardenEditorPage() {
|
||||
ArrowRight: [1, 0],
|
||||
}
|
||||
const commitLater = (fire: () => void) => {
|
||||
nudgeFire.current = fire
|
||||
if (nudgeTimer.current != null) window.clearTimeout(nudgeTimer.current)
|
||||
nudgeTimer.current = window.setTimeout(() => {
|
||||
fire()
|
||||
nudgeTimer.current = null
|
||||
|
gitea-actions
commented
🟡 Object-nudge and plop-nudge branches duplicate the same find-base/compute-next/setLive/commitLater structure maintainability · flagged by 3 models
🪰 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) {
|
||||
|
gitea-actions
commented
🔴 Nudge debounce timer can stomp newer liveObject/livePlanting from a subsequent drag gesture error-handling · flagged by 1 model
🪰 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 : objects.find((o) => o.id === s.selectedId)
|
||||
const base = s.liveObject?.id === s.selectedId ? s.liveObject : objs.find((o) => o.id === s.selectedId)
|
||||
if (!base) return
|
||||
e.preventDefault()
|
||||
|
gitea-actions
commented
🔴 Nudge clamping ignores object rotation, letting plops escape rotated beds error-handling · flagged by 1 model
🪰 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>
|
||||
const next = { ...base, xCm: base.xCm + dir[0] * step, yCm: base.yCm + dir[1] * step }
|
||||
s.setLiveObject(next)
|
||||
s.setLiveObject({ ...base, xCm: base.xCm + dir[0] * step, yCm: base.yCm + dir[1] * step })
|
||||
commitLater(() => {
|
||||
updateObject.mutate({ id: next.id, version: next.version, xCm: next.xCm, yCm: next.yCm })
|
||||
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
|
||||
: plantings.find((p) => p.id === s.selectedPlantingId)
|
||||
s.livePlanting?.id === s.selectedPlantingId ? s.livePlanting : plops.find((p) => p.id === s.selectedPlantingId)
|
||||
if (!base) return
|
||||
e.preventDefault()
|
||||
const obj = objects.find((o) => o.id === base.objectId)
|
||||
const obj = objs.find((o) => o.id === base.objectId)
|
||||
let nx = base.xCm + dir[0] * step
|
||||
let ny = base.yCm + dir[1] * step
|
||||
|
gitea-actions
commented
🔴 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
🪰 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))
|
||||
}
|
||||
const next = { ...base, xCm: nx, yCm: ny }
|
||||
s.setLivePlanting(next)
|
||||
s.setLivePlanting({ ...base, xCm: nx, yCm: ny })
|
||||
commitLater(() => {
|
||||
updatePlanting.mutate({ id: next.id, version: next.version, xCm: next.xCm, yCm: next.yCm })
|
||||
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)
|
||||
})
|
||||
}
|
||||
@@ -180,10 +197,16 @@ export function GardenEditorPage() {
|
||||
window.addEventListener('keydown', onKey)
|
||||
return () => {
|
||||
window.removeEventListener('keydown', onKey)
|
||||
if (nudgeTimer.current != null) window.clearTimeout(nudgeTimer.current)
|
||||
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
|
||||
}, [canEdit, objects, plantings])
|
||||
}, [])
|
||||
|
||||
if (full.isPending) return <p className="p-6 text-sm text-muted">Loading garden…</p>
|
||||
if (full.isError)
|
||||
@@ -243,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">
|
||||
← {garden.name}
|
||||
</button>
|
||||
<span className="max-w-[8rem] truncate text-muted">
|
||||
{focusedObject.name || kindDef(focusedObject.kind)?.label || 'Object'}
|
||||
</span>
|
||||
<span className="max-w-[8rem] truncate text-muted">{objectDisplayName(focusedObject)}</span>
|
||||
{canEdit &&
|
||||
(!focusedObject.plantable ? (
|
||||
<span className="text-xs text-muted">Not plantable</span>
|
||||
@@ -273,18 +294,10 @@ export function GardenEditorPage() {
|
||||
|
||||
{/* Empty-state hints (non-interactive overlays). */}
|
||||
{canEdit && focusedObjectId == null && objects.length === 0 && (
|
||||
<div className="pointer-events-none absolute inset-0 flex items-center justify-center p-6 text-center">
|
||||
<p className="max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur">
|
||||
Pick a shape from the palette, then tap the field to place your first bed.
|
||||
</p>
|
||||
</div>
|
||||
<EditorHint>Pick a shape from the palette, then tap the field to place your first bed.</EditorHint>
|
||||
)}
|
||||
{canEdit && focusedObject?.plantable && focusedPlops.length === 0 && !armedPlant && (
|
||||
<div className="pointer-events-none absolute inset-x-0 top-16 flex justify-center p-6 text-center">
|
||||
<p className="max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur">
|
||||
Tap “+ Add plant”, then tap inside the bed to place plops.
|
||||
</p>
|
||||
</div>
|
||||
<EditorHint position="top">Tap “+ Add plant”, then tap inside the bed to place plops.</EditorHint>
|
||||
)}
|
||||
</div>
|
||||
|
||||
@@ -330,7 +343,7 @@ export function GardenEditorPage() {
|
||||
|
||||
{clearing && focusedObject && (
|
||||
<ClearBedModal
|
||||
objectName={focusedObject.name || kindDef(focusedObject.kind)?.label || 'bed'}
|
||||
objectName={objectDisplayName(focusedObject)}
|
||||
plops={focusedPlops.map((p) => ({ id: p.id, version: p.version }))}
|
||||
gardenId={gid}
|
||||
onClose={() => setClearing(false)}
|
||||
|
||||
🟠 Clear-bed button not disabled for empty plops array, causing no-op confirm
error-handling · flagged by 1 model
ClearBedModal—web/src/editor/ClearBedModal.tsx:35Ifplopsis 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,useClearObjectloops over the array withPromise.all; an empty array resolves immediately, making the UX misleading…🪰 Gadfly · advisory