Fix plant placement + add a Seed Tray (#39) #42

Merged
steve merged 2 commits from fix/plant-placement-and-seed-tray into main 2026-07-19 06:09:51 +00:00
3 changed files with 52 additions and 38 deletions
Showing only changes of commit 30e8d3e56c - Show all commits
+6 -6
View File
@@ -5,17 +5,17 @@ import type { Plant } from '@/lib/plants'
/**
* The Seed Tray strip in the focus-mode toolbar: a row of the garden's
* kept-at-hand plants. Tap a chip to arm that plant for tap-to-place (the armed
* chip is highlighted); ✕ removes it from the tray; the trailing chip opens
* the full catalog picker. See useSeedTray for persistence.
* chip is highlighted); ✕ removes it from the tray; the trailing "+ Plant" chip
* opens the full catalog picker. See useSeedTray for persistence.
*/
export function SeedTray({
plants,
trayPlants,
armedPlantId,
onArm,
onRemove,
onOpenPicker,
}: {
plants: Plant[]
trayPlants: Plant[]
Outdated
Review

Prop named 'plants' is ambiguous inside a SeedTray component; 'trayPlants' would match the hook vocabulary

maintainability · flagged by 1 model

  • web/src/editor/SeedTray.tsx:18 — The prop is named plants, but the parent passes trayPlants and the hook exposes trayPlants. Inside a component literally named SeedTray, plants is ambiguous (could be read as the catalog). Renaming the prop to trayPlants would keep the vocabulary consistent across the seam. Trivial.

🪰 Gadfly · advisory

⚪ **Prop named 'plants' is ambiguous inside a SeedTray component; 'trayPlants' would match the hook vocabulary** _maintainability · flagged by 1 model_ - **`web/src/editor/SeedTray.tsx:18`** — The prop is named `plants`, but the parent passes `trayPlants` and the hook exposes `trayPlants`. Inside a component literally named `SeedTray`, `plants` is ambiguous (could be read as the catalog). Renaming the prop to `trayPlants` would keep the vocabulary consistent across the seam. Trivial. <sub>🪰 Gadfly · advisory</sub>
armedPlantId: number | null
onArm: (plant: Plant) => void
onRemove: (id: number) => void
@@ -23,7 +23,7 @@ export function SeedTray({
}) {
return (
<div className="flex flex-wrap items-center gap-1.5">
{plants.map((p) => {
{trayPlants.map((p) => {
const active = p.id === armedPlantId
return (
<span
Review

Chip styled span wrapping two buttons splits interactive affordance awkwardly

maintainability · flagged by 1 model

  • web/src/editor/SeedTray.tsx:29-54 — Each chip is a styled <span> wrapping two sibling <button>s, with the active-state border/bg on the outer span while each button carries its own rounded-full/focus ring. The interactive affordance is split across a non-interactive wrapper and two buttons; a single button with an inner ✕ (or role="group" on the span) would be cleaner. Not blocking — purely structural readability.

🪰 Gadfly · advisory

⚪ **Chip styled span wrapping two buttons splits interactive affordance awkwardly** _maintainability · flagged by 1 model_ - `web/src/editor/SeedTray.tsx:29-54` — Each chip is a styled `<span>` wrapping two sibling `<button>`s, with the active-state border/bg on the outer span while each button carries its own `rounded-full`/focus ring. The interactive affordance is split across a non-interactive wrapper and two buttons; a single button with an inner ✕ (or `role="group"` on the span) would be cleaner. Not blocking — purely structural readability. <sub>🪰 Gadfly · advisory</sub>
@@ -59,7 +59,7 @@ export function SeedTray({
onClick={onOpenPicker}
className="inline-flex items-center gap-1 rounded-full border border-dashed border-border px-2.5 py-1 text-xs font-medium text-muted outline-none transition-colors hover:border-accent hover:text-accent-strong focus-visible:ring-2 focus-visible:ring-accent/40"
>
Plant
+ Plant
Outdated
Review

Fullwidth + glyph inconsistent with ASCII + used elsewhere

maintainability · flagged by 2 models

  • web/src/editor/SeedTray.tsx:62 — The trailing chip label uses a fullwidth (+ Plant). The prior toolbar button (removed by this diff) used ASCII + Add plant. Inconsistent glyph choice for no functional reason; aligning to + Plant would be consistent and grepable. Trivial.

🪰 Gadfly · advisory

⚪ **Fullwidth + glyph inconsistent with ASCII + used elsewhere** _maintainability · flagged by 2 models_ - `web/src/editor/SeedTray.tsx:62` — The trailing chip label uses a fullwidth `+` (`+ Plant`). The prior toolbar button (removed by this diff) used ASCII `+ Add plant`. Inconsistent glyph choice for no functional reason; aligning to `+ Plant` would be consistent and grepable. Trivial. <sub>🪰 Gadfly · advisory</sub>
</button>
</div>
)
+6 -4
View File
@@ -16,7 +16,8 @@ const TRAY_MAX = 24 // a working set, not a catalog; caps pathological growth
function loadIds(gardenId: number): number[] {
Review

🟠 Unbounded localStorage read bypasses TRAY_MAX cap on load

error-handling, maintainability · flagged by 2 models

  • web/src/lib/seedTray.ts:16 — Unbounded localStorage read, no TRAY_MAX cap on load loadIds filters and returns every numeric ID stored in localStorage, but never applies TRAY_MAX. If the key is corrupted or manually edited to hold thousands of IDs, the component will attempt to render an unbounded number of chips on first mount, degrading or freezing the UI. The TRAY_MAX slice is only enforced inside add, so a malicious/corrupted payload bypasses the safeguard entirely. **Fix:…

🪰 Gadfly · advisory

🟠 **Unbounded localStorage read bypasses TRAY_MAX cap on load** _error-handling, maintainability · flagged by 2 models_ - **`web/src/lib/seedTray.ts:16` — Unbounded `localStorage` read, no `TRAY_MAX` cap on load** `loadIds` filters and returns every numeric ID stored in localStorage, but never applies `TRAY_MAX`. If the key is corrupted or manually edited to hold thousands of IDs, the component will attempt to render an unbounded number of chips on first mount, degrading or freezing the UI. The `TRAY_MAX` slice is only enforced inside `add`, so a malicious/corrupted payload bypasses the safeguard entirely. **Fix:… <sub>🪰 Gadfly · advisory</sub>
try {
const v: unknown = JSON.parse(localStorage.getItem(KEY(gardenId)) ?? '[]')
return Array.isArray(v) ? v.filter((n): n is number => typeof n === 'number') : []
const ids = Array.isArray(v) ? v.filter((n): n is number => typeof n === 'number') : []
return ids.slice(-TRAY_MAX) // honor the cap even if storage was hand-edited
} catch {
return []
}
@@ -30,17 +31,18 @@ function saveIds(gardenId: number, ids: number[]) {
}
}
Review

🟡 SeedTray interface name collides with the SeedTray component in editor/SeedTray.tsx

maintainability · flagged by 2 models

  • web/src/lib/seedTray.ts:33 vs web/src/editor/SeedTray.tsx:11Name collision: SeedTray is both the presentational component (export function SeedTray) and the hook's return type (export interface SeedTray). They live in separate modules so it compiles, but GardenEditorPage.tsx:10 imports the component while the hook's return shape flows in indirectly via useSeedTray, making "SeedTray" ambiguous across searches/stack traces. Renaming the interface (e.g. SeedTrayApi) would d…

🪰 Gadfly · advisory

🟡 **SeedTray interface name collides with the SeedTray component in editor/SeedTray.tsx** _maintainability · flagged by 2 models_ - `web/src/lib/seedTray.ts:33` vs `web/src/editor/SeedTray.tsx:11` — **Name collision**: `SeedTray` is both the presentational component (`export function SeedTray`) and the hook's return type (`export interface SeedTray`). They live in separate modules so it compiles, but `GardenEditorPage.tsx:10` imports the component while the hook's return shape flows in indirectly via `useSeedTray`, making "SeedTray" ambiguous across searches/stack traces. Renaming the interface (e.g. `SeedTrayApi`) would d… <sub>🪰 Gadfly · advisory</sub>
export interface SeedTray {
export interface SeedTrayState {
/** Tray plants in insertion order, resolved against the live catalog. */
trayPlants: Plant[]
/** Add a plant to the end of the tray (no-op if already present). */
/** Add a plant to the end of the tray (no-op if already present; when the tray
Outdated
Review

🟡 add() JSDoc omits the silent FIFO eviction when tray exceeds TRAY_MAX

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **add() JSDoc omits the silent FIFO eviction when tray exceeds TRAY_MAX** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
* is full the oldest entry is evicted). */
add: (plant: Plant) => void
/** Remove a plant from the tray. */
remove: (id: number) => void
}
/** Per-garden Seed Tray backed by localStorage and the plant catalog. */
Review

🟡 useSeedTray doesn't surface plants.isPending/isError, so a catalog fetch failure silently empties the tray with no indication to the user

error-handling · flagged by 1 model

  • web/src/lib/seedTray.ts:44useSeedTray never checks plants.isPending/plants.isError from usePlants() (plants.ts:67-69). If the catalog fetch is loading or fails, byId stays empty and trayPlants silently resolves to [], leaving only the bare "+ Plant" chip with no indication anything went wrong — unlike PlantPicker, which explicitly renders "Loading plants…" and "Couldn't load plants." on isPending/isError (PlantPicker.tsx:136-137). Not data loss (ids stay in `localS…

🪰 Gadfly · advisory

🟡 **useSeedTray doesn't surface plants.isPending/isError, so a catalog fetch failure silently empties the tray with no indication to the user** _error-handling · flagged by 1 model_ - `web/src/lib/seedTray.ts:44` — `useSeedTray` never checks `plants.isPending`/`plants.isError` from `usePlants()` (`plants.ts:67-69`). If the catalog fetch is loading or fails, `byId` stays empty and `trayPlants` silently resolves to `[]`, leaving only the bare "+ Plant" chip with no indication anything went wrong — unlike `PlantPicker`, which explicitly renders "Loading plants…" and "Couldn't load plants." on `isPending`/`isError` (`PlantPicker.tsx:136-137`). Not data loss (ids stay in `localS… <sub>🪰 Gadfly · advisory</sub>
export function useSeedTray(gardenId: number): SeedTray {
export function useSeedTray(gardenId: number): SeedTrayState {
const plants = usePlants()
const [ids, setIds] = useState<number[]>(() => loadIds(gardenId))
+40 -28
View File
1
@@ -244,18 +244,26 @@ export function GardenEditorPage() {
setArmedPlant(plant)
}
// Removing the armed plant from the tray also disarms it, so placement doesn't
// silently continue for a plant the user just cleared out.
function removeFromTrayAndDisarm(id: number) {
removeFromTray(id)
if (armedPlant?.id === id) setArmedPlant(null)
Outdated
Review

🟡 onPickPlant silently adds to tray in both 'place' and 'change' modes; undocumented side-effect

maintainability · flagged by 1 model

  • web/src/pages/GardenEditorPage.tsx:251-261 (onPickPlant) — The function does two things its name and comment don't advertise: it always calls addToTray(plant) (line 253), even in 'change' mode (where the user is swapping an existing plop's plant, not arming for placement). The comment block above it only discusses the resolve-vs-catalog bug; the tray side-effect is silent. A reader has to read the body to learn that "change plant" mutates tray state too. Suggest either documentin…

🪰 Gadfly · advisory

🟡 **onPickPlant silently adds to tray in both 'place' and 'change' modes; undocumented side-effect** _maintainability · flagged by 1 model_ - **`web/src/pages/GardenEditorPage.tsx:251-261` (`onPickPlant`)** — The function does two things its name and comment don't advertise: it *always* calls `addToTray(plant)` (line 253), even in `'change'` mode (where the user is swapping an existing plop's plant, not arming for placement). The comment block above it only discusses the resolve-vs-catalog bug; the tray side-effect is silent. A reader has to read the body to learn that "change plant" mutates tray state too. Suggest either documentin… <sub>🪰 Gadfly · advisory</sub>
}
// The picker hands back the full Plant (from the whole catalog), so use it
// directly rather than re-resolving against the garden's referenced-plant map —
// a not-yet-placed plant isn't in that map, which used to silently abort the
// pick (nothing armed, picker left open).
// pick (nothing armed, picker left open). Picking (place OR change) makes sure
// the plant renders and drops it into this garden's tray for quick reuse.
function onPickPlant(plant: Plant) {
if (!canEdit) return // defense in depth: viewers can't reach the picker anyway
ensurePlant(plant)
addToTray(plant)
if (picker === 'change' && selectedPlop) {
ensurePlant(plant)
updatePlanting.mutate({ id: selectedPlop.id, version: selectedPlop.version, plantId: plant.id })
} else {
armPlant(plant) // 'place' mode: arm for repeat placement
setArmedPlant(plant) // 'place' mode: arm for repeat placement
}
setPicker(null)
}
1
@@ -284,30 +292,34 @@ export function GardenEditorPage() {
{garden.name}
</button>
Outdated
Review

🟠 Removing armed plant from tray doesn't disarm it

error-handling · flagged by 1 model

  • web/src/pages/GardenEditorPage.tsx:293 — Removing an armed plant from the seed tray does not disarm it. If a user taps ✕ on the chip for the currently armed plant, the chip disappears but armedPlant remains set; the user can still place that plant by tapping the bed yet gets no tray indication of what is armed. Wrap removeFromTray so it also calls setArmedPlant(null) when the removed id matches the armed plant.

🪰 Gadfly · advisory

🟠 **Removing armed plant from tray doesn't disarm it** _error-handling · flagged by 1 model_ - **`web/src/pages/GardenEditorPage.tsx:293`** — Removing an armed plant from the seed tray does not disarm it. If a user taps ✕ on the chip for the currently armed plant, the chip disappears but `armedPlant` remains set; the user can still place that plant by tapping the bed yet gets no tray indication of what is armed. Wrap `removeFromTray` so it also calls `setArmedPlant(null)` when the removed id matches the armed plant. <sub>🪰 Gadfly · advisory</sub>
<span className="max-w-[8rem] truncate text-muted">{objectDisplayName(focusedObject)}</span>
{canEdit && !focusedObject.plantable && <span className="text-xs text-muted">Not plantable</span>}
{canEdit && focusedObject.plantable && (
<SeedTray
plants={trayPlants}
armedPlantId={armedPlant?.id ?? null}
onArm={armPlant}
onRemove={removeFromTray}
onOpenPicker={() => setPicker('place')}
/>
)}
{canEdit && focusedObject.plantable && armedPlant && (
<Button variant="ghost" className="px-2 py-1 text-xs" onClick={() => setArmedPlant(null)}>
Done
</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>
)}
{canEdit &&
(focusedObject.plantable ? (
<>
<SeedTray
trayPlants={trayPlants}
armedPlantId={armedPlant?.id ?? null}
onArm={armPlant}
onRemove={removeFromTrayAndDisarm}
onOpenPicker={() => setPicker('place')}
/>
{armedPlant && (
<Button variant="ghost" className="px-2 py-1 text-xs" onClick={() => setArmedPlant(null)}>
Done
</Button>
)}
{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>
)}
</>
) : (
<span className="text-xs text-muted">Not plantable</span>
))}
</div>
)}
<GardenCanvas garden={garden} objects={objects} plantings={plantings} plantsById={plantsById} canEdit={canEdit} />
1
@@ -355,7 +367,7 @@ export function GardenEditorPage() {
<PlantPicker
unit={garden.unitPref}
onClose={() => setPicker(null)}
onSelect={(p) => onPickPlant(p)}
onSelect={onPickPlant}
/>
)}