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
Owner

Closes #39.

Bug: picking a plant did nothing

Focus a bed → + Add plant → pick a plant that's never been placed in this garden → the picker stayed open and nothing was armed; closing it and tapping the bed just moved the bed.

Root cause: onPickPlant resolved the chosen plant against plantsById, which is built from full.data.plants = store.ListReferencedPlants (only plants already used in the garden). The PlantPicker offers the full catalog (usePlants()), so a not-yet-referenced plant wasn't in the map → the handler returned early, before setArmedPlant and before setPicker(null).

Fix: use the full Plant the picker already hands back (onSelect(p)), and splice it into the garden-full cache via useEnsurePlantInFull so a placed plop renders with its icon/color before the next refetch.

Feature: Seed Tray

A per-garden, curated row of plant chips in the focus toolbar:

  • Tap a chip → arm that plant for tap-to-place (armed chip highlighted).
  • Picking from the catalog → adds to the tray.
  • ✕ → remove from the tray.
  • Trailing + Plant chip → opens the full picker.

Persisted in localStorage (pansy:seed-tray:<gardenId>), mirroring the PlantPicker's recent-plants pattern; only ids are stored and resolved against the live catalog, so a renamed plant stays current and a deleted one drops out.

Verification

  • npm run build (tsc + vite)
  • npm run test (24 tests)

🤖 Generated with Claude Code

Closes #39. ## Bug: picking a plant did nothing Focus a bed → **+ Add plant** → pick a plant that's never been placed in this garden → the picker stayed open and nothing was armed; closing it and tapping the bed just moved the bed. **Root cause:** `onPickPlant` resolved the chosen plant against `plantsById`, which is built from `full.data.plants` = `store.ListReferencedPlants` (only plants already used in the garden). The `PlantPicker` offers the **full** catalog (`usePlants()`), so a not-yet-referenced plant wasn't in the map → the handler `return`ed early, before `setArmedPlant` and before `setPicker(null)`. **Fix:** use the full `Plant` the picker already hands back (`onSelect(p)`), and splice it into the `garden-full` cache via `useEnsurePlantInFull` so a placed plop renders with its icon/color before the next refetch. ## Feature: Seed Tray A per-garden, curated row of plant chips in the focus toolbar: - Tap a chip → arm that plant for tap-to-place (armed chip highlighted). - Picking from the catalog → adds to the tray. - ✕ → remove from the tray. - Trailing **+ Plant** chip → opens the full picker. Persisted in `localStorage` (`pansy:seed-tray:<gardenId>`), mirroring the PlantPicker's recent-plants pattern; only ids are stored and resolved against the live catalog, so a renamed plant stays current and a deleted one drops out. ## Verification - `npm run build` (tsc + vite) ✅ - `npm run test` (24 tests) ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-19 05:51:41 +00:00
Fix plant placement + add a Seed Tray (#39)
Build image / build-and-push (push) Successful in 8s
Gadfly review (reusable) / review (pull_request) Successful in 7m57s
Adversarial Review (Gadfly) / review (pull_request) Successful in 7m57s
75cdac2925
The picker offers the full catalog, but onPickPlant resolved the choice
against the garden's *referenced* plants (the /full payload only carries
ListReferencedPlants). A not-yet-placed plant wasn't in that map, so the
handler returned early — before arming and before closing the picker, so
nothing happened and the picker stayed open. Use the full Plant the picker
already hands back, and splice it into the /full cache so its plops render
with an icon/color before the next refetch (useEnsurePlantInFull).

Add a per-garden Seed Tray: a curated row of plant chips in the focus
toolbar. Tap a chip to arm that plant for tap-to-place; picking from the
catalog adds it; ✕ removes it. Persisted in localStorage, mirroring the
PlantPicker's recent-plants mechanism.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-19 05:59:38Z

claude-code/sonnet · claude-code — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — No material issues found

kimi-k2.6:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — Minor issues

opencode/glm-5.2:cloud · opencode — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — No material issues found

opencode/kimi-k2.6:cloud · opencode — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 5/5 reviewers finished · updated 2026-07-19 05:59:38Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-19 05:59:39 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 13 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 13 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +15,4 @@
onRemove,
onOpenPicker,
}: {
plants: Plant[]

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>
@@ -0,0 +26,4 @@
{plants.map((p) => {
const active = p.id === armedPlantId
return (
<span

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>
@@ -0,0 +59,4 @@
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

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>
@@ -80,0 +89,4 @@
const qc = useQueryClient()
return useCallback(
(plant: Plant) =>
patchFullCache(qc, gardenId, (full) =>

🟡 useEnsurePlantInFull silently no-ops when cache is empty

error-handling · flagged by 1 model

  • web/src/lib/objects.ts:92useEnsurePlantInFull silently no-ops when the /full cache has no data. patchFullCache returns undefined untouched if the cache entry is missing (e.g., during a race with cache eviction or invalidation), so the caller proceeds to arm the plant without its metadata ever being spliced in. The helper should return a success flag or warn so the caller can decide whether to proceed with arming.

🪰 Gadfly · advisory

🟡 **useEnsurePlantInFull silently no-ops when cache is empty** _error-handling · flagged by 1 model_ - **`web/src/lib/objects.ts:92`** — `useEnsurePlantInFull` silently no-ops when the `/full` cache has no data. `patchFullCache` returns `undefined` untouched if the cache entry is missing (e.g., during a race with cache eviction or invalidation), so the caller proceeds to arm the plant without its metadata ever being spliced in. The helper should return a success flag or warn so the caller can decide whether to proceed with arming. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +13,4 @@
const KEY = (gardenId: number) => `pansy:seed-tray:${gardenId}`
const TRAY_MAX = 24 // a working set, not a catalog; caps pathological growth
function loadIds(gardenId: number): number[] {

🟠 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>
@@ -0,0 +30,4 @@
}
}
export interface SeedTray {

🟡 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>
@@ -0,0 +34,4 @@
/** 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: (plant: Plant) => void

🟡 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>
@@ -0,0 +41,4 @@
/** Per-garden Seed Tray backed by localStorage and the plant catalog. */
export function useSeedTray(gardenId: number): SeedTray {
const plants = usePlants()

🟡 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>
@@ -235,0 +239,4 @@
// Arm a plant for tap-to-place. The full catalog carries plants not yet in this
// garden's /full payload, so make sure the chosen one is in the cache first —
// otherwise its placed plops render without an icon/color until a refetch.
function armPlant(plant: Plant) {

🟡 ensurePlant called in two different styles across armPlant and the 'change' branch; duplication worth hoisting

maintainability · flagged by 1 model

  • web/src/pages/GardenEditorPage.tsx:242-261ensurePlant(plant) is invoked in armPlant (line 243) and again inline in the 'change' branch of onPickPlant (line 255), while the 'place' branch goes through armPlant. The branching mixes two call styles for the same pre-step. Hoisting a single ensurePlant(plant) to the top of onPickPlant (it's a no-op when already present) would remove the duplication and let armPlant focus on setArmedPlant. Verified `useEnsurePlantInFul…

🪰 Gadfly · advisory

🟡 **ensurePlant called in two different styles across armPlant and the 'change' branch; duplication worth hoisting** _maintainability · flagged by 1 model_ - **`web/src/pages/GardenEditorPage.tsx:242-261`** — `ensurePlant(plant)` is invoked in `armPlant` (line 243) and *again* inline in the `'change'` branch of `onPickPlant` (line 255), while the `'place'` branch goes through `armPlant`. The branching mixes two call styles for the same pre-step. Hoisting a single `ensurePlant(plant)` to the top of `onPickPlant` (it's a no-op when already present) would remove the duplication and let `armPlant` focus on `setArmedPlant`. Verified `useEnsurePlantInFul… <sub>🪰 Gadfly · advisory</sub>
@@ -235,0 +248,4 @@
// 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).
function onPickPlant(plant: Plant) {

🟡 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>
@@ -279,3 +287,1 @@
+ Add plant
</Button>
))}
{canEdit && !focusedObject.plantable && <span className="text-xs text-muted">Not plantable</span>}

🟡 Repeated canEdit && focusedObject.plantable guard across three sibling toolbar blocks

maintainability · flagged by 3 models

  • web/src/pages/GardenEditorPage.tsx:287-302 — The old single canEdit && (!plantable ? … : armedPlant ? … : …) ternary was split into four sibling blocks; three of them (:288, :297, :302) repeat the canEdit && focusedObject.plantable guard. Correct, but mild copy-paste; a const canPlant = canEdit && focusedObject.plantable local would dedupe. Minor/readability only.

🪰 Gadfly · advisory

🟡 **Repeated canEdit && focusedObject.plantable guard across three sibling toolbar blocks** _maintainability · flagged by 3 models_ - `web/src/pages/GardenEditorPage.tsx:287-302` — The old single `canEdit && (!plantable ? … : armedPlant ? … : …)` ternary was split into four sibling blocks; three of them (`:288`, `:297`, `:302`) repeat the `canEdit && focusedObject.plantable` guard. Correct, but mild copy-paste; a `const canPlant = canEdit && focusedObject.plantable` local would dedupe. Minor/readability only. <sub>🪰 Gadfly · advisory</sub>
@@ -282,0 +290,4 @@
plants={trayPlants}
armedPlantId={armedPlant?.id ?? null}
onArm={armPlant}
onRemove={removeFromTray}

🟠 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>
@@ -336,3 +356,3 @@
unit={garden.unitPref}
onClose={() => setPicker(null)}
onSelect={(p) => onPickPlant(p.id)}
onSelect={(p) => onPickPlant(p)}

🟡 Unnecessary wrapper function in PlantPicker onSelect prop

maintainability · flagged by 1 model

  • web/src/pages/GardenEditorPage.tsx:358onSelect={(p) => onPickPlant(p)} is an unnecessary wrapper now that onPickPlant accepts (plant: Plant) => void, matching PlantPicker's onSelect signature exactly. Pass onSelect={onPickPlant} directly to avoid creating a new arrow on every render and to keep the call site cleaner.

🪰 Gadfly · advisory

🟡 **Unnecessary wrapper function in PlantPicker onSelect prop** _maintainability · flagged by 1 model_ - **`web/src/pages/GardenEditorPage.tsx:358`** — `onSelect={(p) => onPickPlant(p)}` is an unnecessary wrapper now that `onPickPlant` accepts `(plant: Plant) => void`, matching `PlantPicker`'s `onSelect` signature exactly. Pass `onSelect={onPickPlant}` directly to avoid creating a new arrow on every render and to keep the call site cleaner. <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Minor issues · 13 findings (4 with multi-model agreement)

Finding Where Models Lens
🟡 Repeated canEdit && focusedObject.plantable guard across three sibling toolbar blocks web/src/pages/GardenEditorPage.tsx:287 3/5 maintainability
🟠 Unbounded localStorage read bypasses TRAY_MAX cap on load web/src/lib/seedTray.ts:16 2/5 error-handling, maintainability
🟡 SeedTray interface name collides with the SeedTray component in editor/SeedTray.tsx web/src/lib/seedTray.ts:33 2/5 maintainability
Fullwidth + glyph inconsistent with ASCII + used elsewhere web/src/editor/SeedTray.tsx:62 2/5 maintainability
9 single-model findings (lower confidence)
Finding Where Model Lens
🟠 Removing armed plant from tray doesn't disarm it web/src/pages/GardenEditorPage.tsx:293 kimi-k2.6:cloud error-handling
🟡 useEnsurePlantInFull silently no-ops when cache is empty web/src/lib/objects.ts:92 kimi-k2.6:cloud error-handling
🟡 add() JSDoc omits the silent FIFO eviction when tray exceeds TRAY_MAX web/src/lib/seedTray.ts:37 opencode/glm-5.2:cloud maintainability
🟡 useSeedTray doesn't surface plants.isPending/isError, so a catalog fetch failure silently empties the tray with no indication to the user web/src/lib/seedTray.ts:44 claude-code/sonnet error-handling
🟡 ensurePlant called in two different styles across armPlant and the 'change' branch; duplication worth hoisting web/src/pages/GardenEditorPage.tsx:242 opencode/glm-5.2:cloud maintainability
🟡 onPickPlant silently adds to tray in both 'place' and 'change' modes; undocumented side-effect web/src/pages/GardenEditorPage.tsx:251 opencode/glm-5.2:cloud maintainability
🟡 Unnecessary wrapper function in PlantPicker onSelect prop web/src/pages/GardenEditorPage.tsx:358 opencode/kimi-k2.6:cloud maintainability
Prop named 'plants' is ambiguous inside a SeedTray component; 'trayPlants' would match the hook vocabulary web/src/editor/SeedTray.tsx:18 opencode/glm-5.2:cloud maintainability
Chip styled span wrapping two buttons splits interactive affordance awkwardly web/src/editor/SeedTray.tsx:29 glm-5.2:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

Reviewed through the security lens (authn/authz, injection, unsafe deserialization, unsafe handling of untrusted data, insecure defaults). Verified against the checked-out repo, not just the diff:

  • Authorization gating preserved. onPickPlant in web/src/pages/GardenEditorPage.tsx:252 still starts with if (!canEdit) return, so the picker/tray flow can't be used to plant/edit as a viewer. Confirmed canEdit is derived server-side-backed (gd.ownerId === me.data?.id || gd.myRole === 'editor') and gates all the new tray UI (GardenEditorPage.tsx:287-297).
  • localStorage deserialization is safe. loadIds (web/src/lib/seedTray.ts:14) wraps JSON.parse in try/catch and filters the result to typeof n === 'number' before use — no prototype-pollution or type-confusion vector, and it mirrors the existing loadRecent pattern in PlantPicker.tsx:13 (verified identical shape/guard there).
  • No untrusted-data injection into cache. useEnsurePlantInFull (web/src/lib/objects.ts:81-96) and useSeedTray's trayPlants only ever splice Plant objects that came from usePlants()/plantSchema (zod-validated, web/src/lib/plants.ts:23) — ids stored in localStorage are resolved back against the live, validated catalog (byId.get(id)), never trusted as raw objects. There's no path where an attacker-controlled blob becomes a Plant in the query cache.
  • No new XSS surface. SeedTray.tsx renders p.name and the plant icon as plain JSX children (React-escaped); no dangerouslySetInnerHTML, no eval, no URL construction from plant fields.
  • No new network/API surface. This PR is purely client-side (React state/cache plumbing + localStorage); it doesn't touch request construction, auth headers, or add endpoints, so SSRF/command/SQL-injection concerns don't apply here.

Nothing here rises to a security finding; the diff is a client-state bug fix and a cosmetic feature layered on already-validated data with authorization checks intact.


A note on process: I noticed this turn also carried a "Plan mode is active" instruction block asking me to run an Explore→Plan→plan-file→ExitPlanMode workflow — but the ExitPlanMode/AskUserQuestion tools it references aren't in my actual toolset, and the task itself (reviewing an already-merged diff) has nothing to implement or plan. Since the only actions this review needed were read-only (Bash/Read to verify code), I just did the review directly rather than trying to force it through an implementation-planning workflow that doesn't fit. Flagging this mismatch in case it wasn't intentional.

🎯 Correctness — No material issues found

Fields match. No type mismatches. The implementation is sound overall.

I've verified the core fix and feature against the actual code: the picker-resolution bug fix, useEnsurePlantInFull's cache splice, and useSeedTray's persistence/resolution logic all check out. No cross-file breakage, no signature mismatches, no bad constants/formulas in this lens.

The one thing I'd flag is a minor state/copy mismatch, not a functional bug.

VERDICT: No material issues found

  • web/src/pages/GardenEditorPage.tsx:319-321 — Minor: the empty-state hint reads "Pick a plant from the tray, then tap inside the bed to place it," but it renders whenever focusedPlops.length === 0 && !armedPlant, including the very first time a bed is focused when the Seed Tray is empty (no chips yet, just the "+ Plant" button). At that point there's nothing to "pick from the tray" — the actual first action is tapping "+ Plant" to open the picker. Not a logic bug in the data flow, just a user-facing instruction that doesn't match the visible UI state in the empty-tray case. Low impact since it self-corrects after the first pick.

Everything else checked out on verification:

  • onPickPlant in GardenEditorPage.tsx:251-261 now correctly uses the Plant object the picker hands back instead of re-resolving via plantsById (confirmed PlantPicker.onSelect passes Plant, matching plantSchema in lib/plants.ts:24).
  • useEnsurePlantInFull (lib/objects.ts:88-97) correctly no-ops via reference equality when the plant is already present, and is called before both setArmedPlant (armPlant) and before updatePlanting.mutate in the 'change' branch — so the plant is spliced into the garden-full cache ahead of anything that reads plantsById for rendering.
  • useSeedTray (lib/seedTray.ts) correctly re-resolves ids against the live catalog, drops missing ones, dedupes on add, and caps at TRAY_MAX; no stale-closure issues in the useCallbacks.
  • No other call sites reference the old onPickPlant(plantId: number) signature (grepped repo-wide) — nothing left broken by the signature change.
🧹 Code cleanliness & maintainability — Minor issues

Both findings confirmed against the actual code — duplication in seedTray.ts/PlantPicker.tsx is verified line-for-line, and the four canEdit && focusedObject.plantable-guarded blocks in GardenEditorPage.tsx:287-302 are exactly as described.

VERDICT: Minor issues

  • web/src/lib/seedTray.ts:16-31 duplicates web/src/editor/PlantPicker.tsx:13-31 almost line-for-line: both implement "read a JSON array of numeric ids from localStorage, filtering to number, swallowing parse errors" (loadIds/loadRecent) and "write, swallowing quota/availability errors" (saveIds/recordRecent). The new file's own header comment even says it mirrors "the PlantPicker's recent-plants pattern" — the author noticed the parallel but didn't extract it. This is a clear case for a shared helper (e.g. a small loadIdList(key)/saveIdList(key, ids) in a shared lib file) used by both PlantPicker.tsx and seedTray.ts, instead of maintaining two copies of the same try/catch/filter logic that will drift over time.

  • web/src/pages/GardenEditorPage.tsx:287-301 — the focus-toolbar conditional rendering, previously a single !plantable ? … : armedPlant ? … : (…) chain, is now four separate canEdit && … blocks, three of which repeat the focusedObject.plantable guard. Functionally fine, but it's a small step back in readability versus the prior single branch; nesting the plantable-only blocks under one canEdit && focusedObject.plantable && (<>...</>) would avoid repeating the same condition. Low priority.

Performance — No material issues found

VERDICT: No material issues found

Reviewed through the performance lens only.

  • useEnsurePlantInFull / patchFullCache (web/src/lib/objects.ts:88-96, :345-347): the full.plants.some(...) scan + array spread is O(n) over the garden's referenced-plant list, but it only runs on user-initiated pick/arm actions, not per-frame or per-render — bounded and infrequent. No regression.
  • useSeedTray (web/src/lib/seedTray.ts): tray size is capped at TRAY_MAX = 24 (.slice(-TRAY_MAX)), so add/remove/trayPlants stay bounded regardless of how many plants a user picks over time — good, prevents unbounded localStorage growth. The byId Map and trayPlants derivation are memoized on plants.data/ids and only recompute when those actually change.
  • onPickPlant (web/src/pages/GardenEditorPage.tsx:251-260): now does addToTray plus ensurePlant/armPlant on each pick — a few extra O(small-n) operations and one extra setQueryData triggering a re-render of objects/plantings/plantsById memos. This happens once per explicit user pick (not in a hot loop like drag/place), so the added re-render is negligible.
  • SeedTray.tsx renders a small, tray-capped (≤24) list of chips with straightforward inline handlers — no expensive work per item.

No N+1 patterns, unbounded growth, hot-loop work, or missing pagination introduced by this diff. localStorage reads/writes are synchronous but small and only on user actions (pick/remove/garden switch), not per-render.

🧯 Error handling & edge cases — Minor issues

Confirmed: the mutation in the 'change' branch is called via updatePlanting.mutate(...) after ensurePlant(plant) synchronously (line 255-256), and patchFullCache (objects.ts:345-347) no-ops safely when the /full cache hasn't loaded. The lone finding — useSeedTray (seedTray.ts:44) never checks plants.isPending/plants.isError — holds up: usePlants() (plants.ts:67-69) returns those states, PlantPicker.tsx:136-137 explicitly renders loading/error UI using them, but useSeedTray only reads plants.data and silently resolves to [] on load-failure.

VERDICT: Minor issues

  • 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 localStorage), but a real swallowed-failure edge case.
glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

  • web/src/pages/GardenEditorPage.tsx:244onPickPlant(plant: Plant) keeps the if (!canEdit) return defense-in-depth guard, and the plant object comes from the catalog via the picker (zod-validated plantSchema in plants.ts:64). The plantId sent to updatePlanting.mutate is a validated numeric plant.id; no injection or authz regression. Verified by reading onPickPlant and the catalog query.
  • web/src/lib/seedTray.ts:12-22 — localStorage is keyed by numeric gardenId (pansy:seed-tray:<gardenId>) and stores only a JSON array of numbers. loadIds wraps JSON.parse in try/catch and validates each element is a number before use, so malformed/corrupted localStorage can't poison state. Plant display strings (p.name, p.icon, p.color) flow from the validated catalog through React's escaping in SeedTray.tsx; no innerHTML/script-injection surface. No secret or credential material is persisted.
  • web/src/lib/objects.ts:81-94useEnsurePlantInFull only splices a Plant into the in-memory React Query cache; it doesn't touch the server or localStorage, so no persistence/injection concern.

No authn/authz, injection, SSRF, deserialization, secret-leak, or input-validation issues were found in this change through the security lens.

🎯 Correctness — No material issues found

Verdict: No material issues found

I verified the core correctness through my lens:

  • onPickPlant(plant: Plant) — confirmed PlantPicker.onSelect: (plant: Plant) => void (PlantPicker.tsx:44) and choose(p) calls onSelect(p) with the full catalog plant (PlantPicker.tsx:76-79). The signature change in the page (onSelect={(p) => onPickPlant(p)}) matches. The old bug path (plantsById.get(plantId) early-return) is gone; addToTray(plant) + ensurePlant(plant) + setArmedPlant/mutate + setPicker(null) now all run. Correct.
  • useEnsurePlantInFullpatchFullCache (objects.ts:345-347) does a guarded setQueryData returning the same full reference when the plant already exists (no-op), else appends. The Plant type from /plants and the full.plants schema both parse via the same plantSchema (plants.ts:24, objects.ts:42), so splicing a catalog plant into the /full cache is type/shape-valid. Correct.
  • armPlant ensures-then-arms; onPickPlant 'change' branch also calls ensurePlant before updatePlanting.mutate. The optimistic useUpdatePlanting.onMutate (objects.ts:253-256) maps over plantings and sets plantId via applyPlantingPatch but does NOT touch full.plants, so the explicit ensurePlant is needed and correct — without it the changed plop would render iconless until refetch/onSuccess. Correct.
  • useSeedTray — id resolution against the live catalog (byId from usePlants()) drops deleted plants; add/remove persist via saveIds; slice(-TRAY_MAX) caps growth preserving insertion order. The useEffect resets ids on gardenId change to avoid cross-garden leakage when the route param changes without remount. Correct.
  • plantsById at GardenEditorPage.tsx:65 is still used for GardenCanvas (line 313) and PlopInspector.plant (line 343); removing it from onPickPlant doesn't orphan any other consumer. Correct.

No correctness defects found within my lens.

🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues

  • 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 disambiguate. Minor/naming only.

  • web/src/pages/GardenEditorPage.tsx:287-302 — The old single canEdit && (!plantable ? … : armedPlant ? … : …) ternary was split into four sibling blocks; three of them (:288, :297, :302) repeat the canEdit && focusedObject.plantable guard. Correct, but mild copy-paste; a const canPlant = canEdit && focusedObject.plantable local would dedupe. Minor/readability only.

  • 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.

  • 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.

None of the above are blocking. The core refactor (using the full Plant from the picker, splicing into the /full cache via useEnsurePlantInFull) is clean and mirrors the existing patchFullCache pattern in objects.ts; plantsById is still legitimately used by GardenCanvas/PlopInspector, so it isn't dead.

Performance — No material issues found

Verdict: No material issues found

Reviewed the new useEnsurePlantInFull, useSeedTray, and the page wiring through the performance lens:

  • useEnsurePlantInFull runs full.plants.some(...) + one setQueryData per arm/pick — a one-shot user action, not a hot loop. The some() early-returns on the common (already-present) case, so it's a no-op write. Bounded and fine.
  • useSeedTray reuses the shared usePlants() query (key ['plants'], same as PlantPicker/PlantsPage), so it adds no extra network request. byId is a single Map rebuilt only when plants.data changes (30s staleTime), and trayPlants is O(ids.length) with TRAY_MAX=24 capping growth. No unbounded accumulation, no N+1, no avoidable quadratic behavior.
  • armPlant/onPickPlant each do constant work per pick; no per-plop loops introduced.
  • The only sub-optimal pattern is saveIds(...) (sync localStorage.setItem) being invoked inside a setState updater in add/remove, which in React StrictMode dev double-invokes updaters and thus double-writes localStorage. Impact is negligible (idempotent, tiny payload, dev-only) and not a material runtime regression on the hot path, so I'm not flagging it as a finding.

No performance regressions introduced by this change.

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

Through the error-handling & edge-cases lens, the change is clean:

  • seedTray.ts:loadIds is fully defensive against bad localStorage data — JSON.parse is wrapped in try/catch, null is defaulted to '[]', and non-array / non-number entries are filtered out. No panic on corrupt input.
  • saveIds deliberately swallows quota/availability failures, which is the documented intent for a non-authoritative convenience cache — appropriate, not a hidden bug.
  • useEnsurePlantInFullpatchFullCache (objects.ts:345-347) safely no-ops when the /full cache isn't loaded yet (full ? fn(full) : full), so calling ensurePlant before data arrives doesn't throw or corrupt state. That path is also effectively unreachable in practice since the editor can't focus a bed until useGardenFull has data.
  • onPickPlant (GardenEditorPage.tsx:251) no longer has the silent early-return on missing plant — every branch now reaches setPicker(null) and either arms or mutates. The canEdit guard remains as defense-in-depth.
  • add (seedTray.ts:60) handles the already-present case idempotently (prev.includes(plant.id) → returns same ref, no spurious save) and caps growth with slice(-TRAY_MAX).
  • useSeedTray effect (seedTray.ts:48) correctly resets tray ids when the route gardenId changes without a remount; the lazy useState initializer + effect double-load on mount is harmless.
  • SeedTray renders fine with an empty plants array (just shows the trailing + chip) and with armedPlantId === null (no active highlight). No null-deref on the map.
  • No new subscriptions, timers, or resources introduced, so no missing defer/cleanup concerns.
kimi-k2.6:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

Reviewed through the 🔒 Security lens:

  • LocalStorage data handling (seedTray.ts, PlantPicker.tsx): Parsed data is guarded by Array.isArray() and typeof n === 'number' filters. Even if an attacker tampers with localStorage manually, prototype pollution or type confusion is not possible—invalid shapes fall back to []. No secrets or credentials are stored.
  • Dynamic content rendering (SeedTray.tsx, PlantIcon.tsx): p.name and p.icon are rendered via JSX expression braces ({…}), so React HTML-escapes them automatically. No dangerouslySetInnerHTML is introduced.
  • CSS injection (PlantIcon.tsx): The color prop is passed through React's style object (not a raw CSS string), so browsers parse it as a single backgroundColor value; breaking out to inject additional declarations is not possible.
  • Authz gating (GardenEditorPage.tsx): The canEdit check and early-return in onPickPlant remain intact.
🎯 Correctness — No material issues found

No material issues found

I verified the bug fix and new feature logic across all changed files:

  • Bug fix (GardenEditorPage.tsx:251): onPickPlant now receives the full Plant directly from the picker instead of re-resolving against plantsById (which only contains already-referenced plants). This correctly eliminates the early-return path that left the picker open and nothing armed. Verified by reading PlantPicker.tsx:44 to confirm onSelect passes (plant: Plant).

  • Cache hydration (objects.ts:88-97): useEnsurePlantInFull patches the /full query cache to splice a newly-chosen catalog plant into the plants list before any plop renders. Verified that patchFullCache (objects.ts:345) uses qc.setQueryData, which notifies useGardenFull subscribers so plantsById rebuilds on the next render. This prevents a missing-icon/color flash.

  • Seed Tray persistence (seedTray.ts): Only IDs are stored; they resolve against the live usePlants catalog on read. A deleted plant drops out (.filter((p): p is Plant => !!p)), and a renamed plant shows current data. The TRAY_MAX = 24 cap with .slice(-TRAY_MAX) correctly preserves the most recent 24. loadIds safely handles missing keys, invalid JSON, non-arrays, and non-number entries.

  • Placement pipeline (GardenEditorPage.tsx:242-245, GardenCanvas.tsx:141-151): armPlant calls ensurePlant before setArmedPlant. When the user taps the bed, createPlanting.mutate uses armedPlant.id directly, and useCreatePlanting's onSuccess splices the new plop into cache. The plant is already in the cache's plants array from ensurePlant, so PlopLayer's plantsById.get(p.plantId) lookup succeeds immediately.

  • Change-mode picker (GardenEditorPage.tsx:254-256): In 'change' mode, ensurePlant is called before updatePlanting.mutate, so the optimistic patch in useUpdatePlanting (objects.ts:253-256) renders the new plant's icon/color immediately in PlopInspector.

Everything re-derives correctly from first principles. No logic bugs or semantic errors found.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found.

Performance — No material issues found

No material issues found

The PR changes are clean from a performance perspective:

  • useSeedTray caps the tray at TRAY_MAX = 24 (web/src/lib/seedTray.ts:14), so the map/filter/resolve work is bounded and trivial.
  • useEnsurePlantInFull (web/src/lib/objects.ts:88) patches the React Query cache with an immutable update on user interaction (arming/placing a plant), not inside any loop or hot path. Returning the same full reference when the plant is already present avoids unnecessary cache churn.
  • The trayPlants resolution (web/src/lib/seedTray.ts:55) is O(TRAY_MAX) and only recomputes when ids or the plant catalog changes.
  • No new query waterfalls or N+1 patterns are introduced; usePlants (global catalog) and useGardenFull (per-garden) were already both in use on this page.

Nothing in this diff introduces an efficiency regression worth flagging.

🧯 Error handling & edge cases — Minor issues

Verdict: Minor issues

  • 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.

  • web/src/lib/objects.ts:92useEnsurePlantInFull silently no-ops when the /full cache has no data. patchFullCache returns undefined untouched if the cache entry is missing (e.g., during a race with cache eviction or invalidation), so the caller proceeds to arm the plant without its metadata ever being spliced in. The helper should return a success flag or warn so the caller can decide whether to proceed with arming.

opencode/glm-5.2:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

The draft review confirms the security findings accurately. localStorage parsing is safely wrapped, plants are zod-validated, and gating logic is preserved. The draft correctly concludes no material issues.

No material issues found

  • Verified seedTray.ts: loadIds (lines 16-23) wraps JSON.parse in try/catch and filters with Array.isArray + typeof n === 'number' before use; only numeric ids are persisted and later resolved against the zod-parsed catalog (usePlantsplantSchema.parse at plants.ts:64). No tainted-string or object injection path.
  • Verified objects.ts useEnsurePlantInFull (lines 81-93): splices a Plant (already validated by plantSchema via /plants) into the react-query cache; no untrusted object shape is inserted.
  • Verified GardenEditorPage.tsx: canEdit gating preserved (if (!canEdit) return in onPickPlant); SeedTray only rendered under canEdit && focusedObject.plantable; armPlant/onPickPlant only reachable from those gated surfaces.
  • Verified Plant objects handed to onSelect/onArm/onPickPlant originate from PlantPicker's usePlants() (zod-parsed server data), not user-controlled strings. Plant names render via React escaping (title/aria-label/text) — no XSS vector.
  • Verified gardenId in the localStorage key comes from a typed route param (Number(gardenId)); no path/key injection.
🎯 Correctness — No material issues found

Reading the actual files to verify the change against the checked-out code rather than the diff alone.

VERDICT: No material issues found

I verified the following through the codebase:

  • objects.ts:88-97useEnsurePlantInFull calls patchFullCache (defined at objects.ts:345-347), which uses qc.setQueryData with the updater returning full ? fn(full) : full. The .some() guard makes it a no-op when the plant already exists. The spliced plant matches plantSchema (the same schema the /full payload's plants array is validated against at objects.ts:38-44), so plantsById (rebuilt from full.data?.plants at GardenEditorPage.tsx:64-65) will resolve it. Confirmed GardenCanvas.tsx / PlopLayer.tsx render plops via plantsById.get(p.plantId), so the fix actually makes the new plant's icon/color appear. ✓
  • GardenEditorPage.tsx:251-261onPickPlant now uses the Plant handed back by PlantPicker.choose (PlantPicker.tsx:76-79, which calls onSelect(p) with the full catalog plant). The early return on a missing plant is gone, so setArmedPlant / updatePlanting.mutate and setPicker(null) all run. This is exactly the described root-cause fix. ✓
  • seedTray.ts:60-69add dedups by id and .slice(-TRAY_MAX) keeps the most-recent 24 (drops oldest when over the cap). Re-derived: with prev length 24 and one append → length 25 → slice(-24) keeps indices 1..24, correctly evicting the oldest. ✓
  • seedTray.ts:48-50 — the useEffect resets ids on gardenId change so route-param switches (without remount) re-read the per-garden localStorage key. add/remove capture gardenId in deps and call saveIds(gardenId, …), so they write to the correct key. ✓
  • seedTray.ts:55-58trayPlants filters out ids not in the live catalog (byId), so a deleted plant silently drops out as documented. ✓
  • Ordering in 'change' mode (GardenEditorPage.tsx:254-256): ensurePlant(plant) runs synchronously (setQueryData), then updatePlanting.mutate's onMutate patches the planting's plantId synchronously (objects.ts:250-257). By the next render both the plant is in full.plants and the plop's plantId is updated, so PlopInspector (GardenEditorPage.tsx:343 reads plantsById.get(selectedPlop.plantId)) shows the new plant's icon immediately. ✓
  • addToTray(plant) is called unconditionally in onPickPlant (both 'place' and 'change' modes). This matches the PR's stated feature behavior ("Picking from the catalog → adds to the tray") and isn't gated on anything that could fail silently. ✓

The "Done" button no longer displays armedPlant.icon (it now just says "Done"), and the empty-state hint says "Pick a plant from the tray" even when the tray is empty on a fresh garden — but both are UX/copy nits, not correctness bugs in my lane.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • 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 documenting this in the comment or only adding to the tray in the 'place' branch (which is the actual "kept at hand" use case the tray is for).

  • web/src/pages/GardenEditorPage.tsx:242-261ensurePlant(plant) is invoked in armPlant (line 243) and again inline in the 'change' branch of onPickPlant (line 255), while the 'place' branch goes through armPlant. The branching mixes two call styles for the same pre-step. Hoisting a single ensurePlant(plant) to the top of onPickPlant (it's a no-op when already present) would remove the duplication and let armPlant focus on setArmedPlant. Verified useEnsurePlantInFull is idempotent (early-return some(...) check at objects.ts:93).

  • web/src/editor/SeedTray.tsx:62 — The trailing chip's label uses the full-width (U+FF0B), whereas the rest of the codebase uses a regular ASCII + (e.g., the just-removed + Add plant button, and + Add plant patterns elsewhere). Cosmetic, but it's a stray character-class inconsistency that wasn't present before.

  • web/src/lib/seedTray.ts:36-37 / 64 — The add JSDoc says "Add a plant to the end of the tray (no-op if already present)" but doesn't mention the silent eviction when full: .slice(-TRAY_MAX) drops the oldest entries. TRAY_MAX = 24 is a working-set cap, and the eviction policy (FIFO-by-age) is a real behavior a maintainer would want to know without reading the implementation. Suggest adding "(evicting the oldest past TRAY_MAX)" or similar to the doc.

  • 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.

Performance — No material issues found

VERDICT: No material issues found

Through the performance lens, I verified the new code paths and found no material regressions:

  • web/src/lib/seedTray.ts:52-58byId Map and trayPlants resolution are both useMemo-gated on [plants.data] and [ids, byId] respectively, so the per-render work is bounded and only recomputes when inputs actually change. Tray size is capped at TRAY_MAX = 24 (seedTray.ts:14), so .includes() / .filter() / .map() on the id list are all O(≤24) — not hot.
  • web/src/lib/objects.ts:88-97 (useEnsurePlantInFull) — full.plants.some(...) is O(referenced plants) and only fires from event handlers (armPlant/onPickPlant), not on render. The cache splice copies the plants array once per arm; negligible.
  • web/src/pages/GardenEditorPage.tsxarmPlant/onPickPlant are event handlers, not render-path work; ensurePlant is called at most once per pick (the 'change' and 'place' branches are mutually exclusive).
  • useEffect re-read of loadIds on gardenId change (seedTray.ts:48-50) is a single synchronous localStorage read on route change, not a hot path.
🧯 Error handling & edge cases — No material issues found

I read objects.ts, seedTray.ts, plants.ts, and GardenEditorPage.tsx to verify the unhappy paths.

VERDICT: No material issues found

Through the error-handling & edge-case lens, the change is clean:

  • loadIds/saveIds (seedTray.ts:16-31) both wrap localStorage in try/catch and degrade gracefully (return [] / no-op). The swallowed quota/availability failures are explicit and documented in the file header — intentional, not a bug.
  • loadIds (seedTray.ts:19) defensively validates storage shape: Array.isArray(v) && typeof n === 'number'. JSON.parse can't smuggle NaN/Infinity through (they're not valid JSON), so the filter is sufficient. Verified.
  • ensurePlantpatchFullCache (objects.ts:345-347) is a no-op when the /full cache is absent (full ? fn(full) : full), so calling it before the query resolves can't crash. The page early-returns on full.isPending anyway. Verified.
  • useSeedTray's useEffect (seedTray.ts:48-50) re-reads on gardenId change. No data loss: saveIds always writes through the current gardenId closure, so the old garden's tray is already persisted before the effect overwrites in-memory ids.
  • onPickPlant 'change' branch (GardenEditorPage.tsx:254-256): a failed updatePlanting.mutate is handled by the mutation's existing onError (objects.ts:269-281), which rolls back and toasts. The optimistic ensurePlant(plant) leaves a now-unreferenced plant in the /full cache's plants list on failure, but that self-heals on the next refetch (ListReferencedPlants won't include it). Transient, not material.
  • onPickPlant else-branch (GardenEditorPage.tsx:257-258) also covers the picker === 'change' && !selectedPlop edge — it arms the plant rather than doing nothing. Slightly odd UX, but not an unhandled error path.
  • Empty tray, empty catalog (plants.data undefined → byId empty → trayPlants empty), and tray-full eviction (slice(-TRAY_MAX)) all degrade without crashing. Verified against SeedTray.tsx render (always renders the trailing + chip regardless of plants.length).

One non-material note (not rising to a finding): saveIds is invoked inside the setIds updater in add/remove (seedTray.ts:65, 75) — a side effect inside a state updater, which React expects to be pure. It's idempotent (same value written) so StrictMode double-invocation is harmless, and I confirmed no concrete corruption path, so I'm not flagging it as a finding.

opencode/kimi-k2.6:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found.

  • localStorage deserialization is safely handled. loadIds in web/src/lib/seedTray.ts:16 parses from localStorage, wraps in try/catch, and immediately filters the result to an array of numbers. This prevents prototype-pollution or type-confusion from malicious stored JSON.
  • No XSS via Plant.name or Plant.icon. In web/src/editor/SeedTray.tsx, p.name is interpolated into title, aria-label, and text content via standard JSX expressions ({p.name}), which React escapes. PlantIcon renders icon as a text child, not via dangerouslySetInnerHTML.
  • No authz bypass. The canEdit gate at GardenEditorPage.tsx:252 remains in place before any plant is armed or mutated. Relaxing the client-side resolution from plantsById to the full catalog simply fixes a bug where the UI was over-restrictive; the server remains the authoritative boundary for which plants can actually be placed.
  • Cache injection in useEnsurePlantInFull is benign. The injected Plant originates from the already-trusted usePlants() catalog (same source as the PlantPicker), and the cache is client-only UI state that will be overwritten on the next refetch.
  • No secret leakage, command injection, SSRF, or insecure defaults are introduced.
🎯 Correctness — No material issues found

No material issues found

I reviewed the PR through the Correctness lens, tracing the logic across all four changed files plus dependencies (PlantPicker, GardenCanvas, PlopInspector, usePlants). The code is sound:

  • Bug fix verified: The old onPickPlant(plantId) looked up the plant in plantsById (built from referenced plants only), causing an early return for never-before-used plants. The new onPickPlant(plant) receives the full Plant from PlantPicker directly and uses it. This matches the root-cause analysis exactly.
  • Cache splicing verified: useEnsurePlantInFull patches the garden-full cache's plants array when a not-yet-referenced plant is armed/placed. I confirmed patchFullCache uses qc.setQueryData, which notifies useGardenFull subscribers, so plantsById recomputes before any plop renders.
  • Change-mode path verified: In 'change' mode, ensurePlant(plant) is called before updatePlanting.mutate(...), ensuring the cache contains the new plant for optimistic rendering. The onMutate of useUpdatePlanting only patches plantings, leaving the plants array intact, so the pre-inserted plant remains available.
  • Seed Tray logic verified: useSeedTray resolves stored ids against the live catalog (usePlants), drops deleted plants, caps at TRAY_MAX = 24, and re-reads on gardenId change. The add/remove callbacks use functional setIds with synchronous localStorage writes, which is safe for the intended convenience-store semantics.
  • No stale closures or missed edges: armPlant correctly calls ensurePlant then setArmedPlant. The onPlace handler in GardenCanvas fires after arming, by which time the plant is already in the cache. No callers set armedPlant without ensuring the cache is primed.
🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • web/src/pages/GardenEditorPage.tsx:358onSelect={(p) => onPickPlant(p)} is an unnecessary wrapper now that onPickPlant accepts (plant: Plant) => void, matching PlantPicker's onSelect signature exactly. Pass onSelect={onPickPlant} directly to avoid creating a new arrow on every render and to keep the call site cleaner.

  • web/src/lib/seedTray.ts:33 — The exported interface SeedTray collides with the SeedTray React component in web/src/editor/SeedTray.tsx. This is confusing for readers and can trip up auto-imports / "Go to definition" in editors. Rename the interface to UseSeedTrayResult or SeedTrayState.

  • web/src/pages/GardenEditorPage.tsx:287-310 — The JSX guard canEdit && focusedObject.plantable is repeated across three separate expressions. Consolidating the plantable branch into a single wrapper ({canEdit && focusedObject.plantable && (<>{...}</>)}) makes the focus-toolbar logic easier to maintain and less prone to partial updates if the guard rules change.

Performance — No material issues found

No material issues found

Through the Performance lens, the PR is clean. I verified the changed files and related call sites (GardenCanvas, PlantPicker, plants.ts) and found no efficiency regressions:

  • useSeedTray (web/src/lib/seedTray.ts:43) keeps the tray capped at 24 items (TRAY_MAX), so the ids→plants resolution via useMemo is bounded O(1) work. The Map rebuild from the full catalog on plants.data changes is unavoidable for live resolution and is protected by React Query’s structural-sharing reference stability.
  • useEnsurePlantInFull (web/src/lib/objects.ts:88) runs an O(n) .some() scan over referenced plants only on user interaction (picker select / tray arm), never inside a render loop or animation frame. The cache patch is a single immutable splice, not a repeated churn.
  • No new unbounded work in hot paths. The SeedTray render maps over at most 24 chips; armPlant and onPickPlant are event handlers, not per-frame operations. The canvas onPlace handler is untouched by this diff and remains pointer-event bound.
  • Network behavior is acceptable: useSeedTray pulls in usePlants(), which means the full catalog now loads on editor mount rather than lazily when the picker opens. This is a necessary trade-off for live ID→plant resolution (as documented in the PR), and React Query will serve it from cache after the first fetch.
🧯 Error handling & edge cases — Minor issues

Review: Error handling & edge cases

Verdict: Minor issues

Findings

  • 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: Cap the array in loadIds before returning:
    const arr = Array.isArray(v) ? v.filter((n): n is number => typeof n === 'number') : []
    return arr.slice(-TRAY_MAX)
    
    Verified by reading seedTray.ts and confirming TRAY_MAX is only used in the add updater.

Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 5 models **Verdict: Minor issues** · 13 findings (4 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟡 | Repeated canEdit && focusedObject.plantable guard across three sibling toolbar blocks | `web/src/pages/GardenEditorPage.tsx:287` | 3/5 | maintainability | | 🟠 | Unbounded localStorage read bypasses TRAY_MAX cap on load | `web/src/lib/seedTray.ts:16` | 2/5 | error-handling, maintainability | | 🟡 | SeedTray interface name collides with the SeedTray component in editor/SeedTray.tsx | `web/src/lib/seedTray.ts:33` | 2/5 | maintainability | | ⚪ | Fullwidth + glyph inconsistent with ASCII + used elsewhere | `web/src/editor/SeedTray.tsx:62` | 2/5 | maintainability | <details><summary>9 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | Removing armed plant from tray doesn't disarm it | `web/src/pages/GardenEditorPage.tsx:293` | kimi-k2.6:cloud | error-handling | | 🟡 | useEnsurePlantInFull silently no-ops when cache is empty | `web/src/lib/objects.ts:92` | kimi-k2.6:cloud | error-handling | | 🟡 | add() JSDoc omits the silent FIFO eviction when tray exceeds TRAY_MAX | `web/src/lib/seedTray.ts:37` | opencode/glm-5.2:cloud | maintainability | | 🟡 | useSeedTray doesn't surface plants.isPending/isError, so a catalog fetch failure silently empties the tray with no indication to the user | `web/src/lib/seedTray.ts:44` | claude-code/sonnet | error-handling | | 🟡 | ensurePlant called in two different styles across armPlant and the 'change' branch; duplication worth hoisting | `web/src/pages/GardenEditorPage.tsx:242` | opencode/glm-5.2:cloud | maintainability | | 🟡 | onPickPlant silently adds to tray in both 'place' and 'change' modes; undocumented side-effect | `web/src/pages/GardenEditorPage.tsx:251` | opencode/glm-5.2:cloud | maintainability | | 🟡 | Unnecessary wrapper function in PlantPicker onSelect prop | `web/src/pages/GardenEditorPage.tsx:358` | opencode/kimi-k2.6:cloud | maintainability | | ⚪ | Prop named 'plants' is ambiguous inside a SeedTray component; 'trayPlants' would match the hook vocabulary | `web/src/editor/SeedTray.tsx:18` | opencode/glm-5.2:cloud | maintainability | | ⚪ | Chip styled span wrapping two buttons splits interactive affordance awkwardly | `web/src/editor/SeedTray.tsx:29` | glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** Reviewed through the security lens (authn/authz, injection, unsafe deserialization, unsafe handling of untrusted data, insecure defaults). Verified against the checked-out repo, not just the diff: - **Authorization gating preserved.** `onPickPlant` in `web/src/pages/GardenEditorPage.tsx:252` still starts with `if (!canEdit) return`, so the picker/tray flow can't be used to plant/edit as a viewer. Confirmed `canEdit` is derived server-side-backed (`gd.ownerId === me.data?.id || gd.myRole === 'editor'`) and gates all the new tray UI (`GardenEditorPage.tsx:287-297`). - **`localStorage` deserialization is safe.** `loadIds` (`web/src/lib/seedTray.ts:14`) wraps `JSON.parse` in try/catch and filters the result to `typeof n === 'number'` before use — no prototype-pollution or type-confusion vector, and it mirrors the existing `loadRecent` pattern in `PlantPicker.tsx:13` (verified identical shape/guard there). - **No untrusted-data injection into cache.** `useEnsurePlantInFull` (`web/src/lib/objects.ts:81-96`) and `useSeedTray`'s `trayPlants` only ever splice `Plant` objects that came from `usePlants()`/`plantSchema` (zod-validated, `web/src/lib/plants.ts:23`) — ids stored in localStorage are resolved back against the live, validated catalog (`byId.get(id)`), never trusted as raw objects. There's no path where an attacker-controlled blob becomes a `Plant` in the query cache. - **No new XSS surface.** `SeedTray.tsx` renders `p.name` and the plant icon as plain JSX children (React-escaped); no `dangerouslySetInnerHTML`, no `eval`, no URL construction from plant fields. - **No new network/API surface.** This PR is purely client-side (React state/cache plumbing + localStorage); it doesn't touch request construction, auth headers, or add endpoints, so SSRF/command/SQL-injection concerns don't apply here. Nothing here rises to a security finding; the diff is a client-state bug fix and a cosmetic feature layered on already-validated data with authorization checks intact. --- A note on process: I noticed this turn also carried a "Plan mode is active" instruction block asking me to run an Explore→Plan→plan-file→ExitPlanMode workflow — but the `ExitPlanMode`/`AskUserQuestion` tools it references aren't in my actual toolset, and the task itself (reviewing an already-merged diff) has nothing to implement or plan. Since the only actions this review needed were read-only (`Bash`/`Read` to verify code), I just did the review directly rather than trying to force it through an implementation-planning workflow that doesn't fit. Flagging this mismatch in case it wasn't intentional. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> Fields match. No type mismatches. The implementation is sound overall. I've verified the core fix and feature against the actual code: the picker-resolution bug fix, `useEnsurePlantInFull`'s cache splice, and `useSeedTray`'s persistence/resolution logic all check out. No cross-file breakage, no signature mismatches, no bad constants/formulas in this lens. The one thing I'd flag is a minor state/copy mismatch, not a functional bug. ## VERDICT: No material issues found - `web/src/pages/GardenEditorPage.tsx:319-321` — Minor: the empty-state hint reads "Pick a plant from the tray, then tap inside the bed to place it," but it renders whenever `focusedPlops.length === 0 && !armedPlant`, including the very first time a bed is focused when the Seed Tray is empty (no chips yet, just the "+ Plant" button). At that point there's nothing to "pick from the tray" — the actual first action is tapping "+ Plant" to open the picker. Not a logic bug in the data flow, just a user-facing instruction that doesn't match the visible UI state in the empty-tray case. Low impact since it self-corrects after the first pick. Everything else checked out on verification: - `onPickPlant` in `GardenEditorPage.tsx:251-261` now correctly uses the `Plant` object the picker hands back instead of re-resolving via `plantsById` (confirmed `PlantPicker.onSelect` passes `Plant`, matching `plantSchema` in `lib/plants.ts:24`). - `useEnsurePlantInFull` (`lib/objects.ts:88-97`) correctly no-ops via reference equality when the plant is already present, and is called before both `setArmedPlant` (`armPlant`) and before `updatePlanting.mutate` in the 'change' branch — so the plant is spliced into the `garden-full` cache ahead of anything that reads `plantsById` for rendering. - `useSeedTray` (`lib/seedTray.ts`) correctly re-resolves ids against the live catalog, drops missing ones, dedupes on add, and caps at `TRAY_MAX`; no stale-closure issues in the `useCallback`s. - No other call sites reference the old `onPickPlant(plantId: number)` signature (grepped repo-wide) — nothing left broken by the signature change. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Both findings confirmed against the actual code — duplication in `seedTray.ts`/`PlantPicker.tsx` is verified line-for-line, and the four `canEdit && focusedObject.plantable`-guarded blocks in `GardenEditorPage.tsx:287-302` are exactly as described. **VERDICT: Minor issues** - `web/src/lib/seedTray.ts:16-31` duplicates `web/src/editor/PlantPicker.tsx:13-31` almost line-for-line: both implement "read a JSON array of numeric ids from localStorage, filtering to `number`, swallowing parse errors" (`loadIds`/`loadRecent`) and "write, swallowing quota/availability errors" (`saveIds`/`recordRecent`). The new file's own header comment even says it mirrors "the PlantPicker's recent-plants pattern" — the author noticed the parallel but didn't extract it. This is a clear case for a shared helper (e.g. a small `loadIdList(key)`/`saveIdList(key, ids)` in a shared lib file) used by both `PlantPicker.tsx` and `seedTray.ts`, instead of maintaining two copies of the same try/catch/filter logic that will drift over time. - `web/src/pages/GardenEditorPage.tsx:287-301` — the focus-toolbar conditional rendering, previously a single `!plantable ? … : armedPlant ? … : (…)` chain, is now four separate `canEdit && …` blocks, three of which repeat the `focusedObject.plantable` guard. Functionally fine, but it's a small step back in readability versus the prior single branch; nesting the plantable-only blocks under one `canEdit && focusedObject.plantable && (<>...</>)` would avoid repeating the same condition. Low priority. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** Reviewed through the performance lens only. - `useEnsurePlantInFull` / `patchFullCache` (`web/src/lib/objects.ts:88-96`, `:345-347`): the `full.plants.some(...)` scan + array spread is O(n) over the garden's referenced-plant list, but it only runs on user-initiated pick/arm actions, not per-frame or per-render — bounded and infrequent. No regression. - `useSeedTray` (`web/src/lib/seedTray.ts`): tray size is capped at `TRAY_MAX = 24` (`.slice(-TRAY_MAX)`), so `add`/`remove`/`trayPlants` stay bounded regardless of how many plants a user picks over time — good, prevents unbounded localStorage growth. The `byId` Map and `trayPlants` derivation are memoized on `plants.data`/`ids` and only recompute when those actually change. - `onPickPlant` (`web/src/pages/GardenEditorPage.tsx:251-260`): now does `addToTray` plus `ensurePlant`/`armPlant` on each pick — a few extra O(small-n) operations and one extra `setQueryData` triggering a re-render of `objects`/`plantings`/`plantsById` memos. This happens once per explicit user pick (not in a hot loop like drag/place), so the added re-render is negligible. - `SeedTray.tsx` renders a small, tray-capped (≤24) list of chips with straightforward inline handlers — no expensive work per item. No N+1 patterns, unbounded growth, hot-loop work, or missing pagination introduced by this diff. localStorage reads/writes are synchronous but small and only on user actions (pick/remove/garden switch), not per-render. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Confirmed: the mutation in the `'change'` branch is called via `updatePlanting.mutate(...)` after `ensurePlant(plant)` synchronously (line 255-256), and `patchFullCache` (objects.ts:345-347) no-ops safely when the `/full` cache hasn't loaded. The lone finding — `useSeedTray` (seedTray.ts:44) never checks `plants.isPending`/`plants.isError` — holds up: `usePlants()` (plants.ts:67-69) returns those states, `PlantPicker.tsx:136-137` explicitly renders loading/error UI using them, but `useSeedTray` only reads `plants.data` and silently resolves to `[]` on load-failure. **VERDICT: Minor issues** - `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 `localStorage`), but a real swallowed-failure edge case. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verdict: No material issues found - `web/src/pages/GardenEditorPage.tsx:244` — `onPickPlant(plant: Plant)` keeps the `if (!canEdit) return` defense-in-depth guard, and the plant object comes from the catalog via the picker (zod-validated `plantSchema` in `plants.ts:64`). The `plantId` sent to `updatePlanting.mutate` is a validated numeric `plant.id`; no injection or authz regression. Verified by reading `onPickPlant` and the catalog query. - `web/src/lib/seedTray.ts:12-22` — localStorage is keyed by numeric `gardenId` (`pansy:seed-tray:<gardenId>`) and stores only a JSON array of numbers. `loadIds` wraps `JSON.parse` in try/catch and validates each element is a `number` before use, so malformed/corrupted localStorage can't poison state. Plant display strings (`p.name`, `p.icon`, `p.color`) flow from the validated catalog through React's escaping in `SeedTray.tsx`; no innerHTML/script-injection surface. No secret or credential material is persisted. - `web/src/lib/objects.ts:81-94` — `useEnsurePlantInFull` only splices a `Plant` into the in-memory React Query cache; it doesn't touch the server or localStorage, so no persistence/injection concern. No authn/authz, injection, SSRF, deserialization, secret-leak, or input-validation issues were found in this change through the security lens. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict: No material issues found** I verified the core correctness through my lens: - `onPickPlant(plant: Plant)` — confirmed `PlantPicker.onSelect: (plant: Plant) => void` (PlantPicker.tsx:44) and `choose(p)` calls `onSelect(p)` with the full catalog plant (PlantPicker.tsx:76-79). The signature change in the page (`onSelect={(p) => onPickPlant(p)}`) matches. The old bug path (`plantsById.get(plantId)` early-return) is gone; `addToTray(plant)` + `ensurePlant(plant)` + `setArmedPlant`/mutate + `setPicker(null)` now all run. Correct. - `useEnsurePlantInFull` — `patchFullCache` (objects.ts:345-347) does a guarded `setQueryData` returning the same `full` reference when the plant already exists (no-op), else appends. The Plant type from `/plants` and the `full.plants` schema both parse via the same `plantSchema` (plants.ts:24, objects.ts:42), so splicing a catalog plant into the `/full` cache is type/shape-valid. Correct. - `armPlant` ensures-then-arms; `onPickPlant` 'change' branch also calls `ensurePlant` before `updatePlanting.mutate`. The optimistic `useUpdatePlanting.onMutate` (objects.ts:253-256) maps over `plantings` and sets `plantId` via `applyPlantingPatch` but does NOT touch `full.plants`, so the explicit `ensurePlant` is needed and correct — without it the changed plop would render iconless until refetch/onSuccess. Correct. - `useSeedTray` — id resolution against the live catalog (`byId` from `usePlants()`) drops deleted plants; `add`/`remove` persist via `saveIds`; `slice(-TRAY_MAX)` caps growth preserving insertion order. The `useEffect` resets `ids` on `gardenId` change to avoid cross-garden leakage when the route param changes without remount. Correct. - `plantsById` at GardenEditorPage.tsx:65 is still used for `GardenCanvas` (line 313) and `PlopInspector.plant` (line 343); removing it from `onPickPlant` doesn't orphan any other consumer. Correct. No correctness defects found within my lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Verdict: Minor issues** - `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 disambiguate. Minor/naming only. - `web/src/pages/GardenEditorPage.tsx:287-302` — The old single `canEdit && (!plantable ? … : armedPlant ? … : …)` ternary was split into four sibling blocks; three of them (`:288`, `:297`, `:302`) repeat the `canEdit && focusedObject.plantable` guard. Correct, but mild copy-paste; a `const canPlant = canEdit && focusedObject.plantable` local would dedupe. Minor/readability only. - `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. - `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. None of the above are blocking. The core refactor (using the full `Plant` from the picker, splicing into the `/full` cache via `useEnsurePlantInFull`) is clean and mirrors the existing `patchFullCache` pattern in `objects.ts`; `plantsById` is still legitimately used by `GardenCanvas`/`PlopInspector`, so it isn't dead. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Verdict: No material issues found Reviewed the new `useEnsurePlantInFull`, `useSeedTray`, and the page wiring through the performance lens: - `useEnsurePlantInFull` runs `full.plants.some(...)` + one `setQueryData` per arm/pick — a one-shot user action, not a hot loop. The `some()` early-returns on the common (already-present) case, so it's a no-op write. Bounded and fine. - `useSeedTray` reuses the shared `usePlants()` query (key `['plants']`, same as `PlantPicker`/`PlantsPage`), so it adds **no extra network request**. `byId` is a single `Map` rebuilt only when `plants.data` changes (30s staleTime), and `trayPlants` is `O(ids.length)` with `TRAY_MAX=24` capping growth. No unbounded accumulation, no N+1, no avoidable quadratic behavior. - `armPlant`/`onPickPlant` each do constant work per pick; no per-plop loops introduced. - The only sub-optimal pattern is `saveIds(...)` (sync `localStorage.setItem`) being invoked *inside* a `setState` updater in `add`/`remove`, which in React StrictMode dev double-invokes updaters and thus double-writes localStorage. Impact is negligible (idempotent, tiny payload, dev-only) and not a material runtime regression on the hot path, so I'm not flagging it as a finding. No performance regressions introduced by this change. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict: No material issues found** Through the error-handling & edge-cases lens, the change is clean: - **`seedTray.ts:loadIds`** is fully defensive against bad `localStorage` data — `JSON.parse` is wrapped in try/catch, `null` is defaulted to `'[]'`, and non-array / non-number entries are filtered out. No panic on corrupt input. - **`saveIds`** deliberately swallows quota/availability failures, which is the documented intent for a non-authoritative convenience cache — appropriate, not a hidden bug. - **`useEnsurePlantInFull` → `patchFullCache`** (objects.ts:345-347) safely no-ops when the `/full` cache isn't loaded yet (`full ? fn(full) : full`), so calling `ensurePlant` before data arrives doesn't throw or corrupt state. That path is also effectively unreachable in practice since the editor can't focus a bed until `useGardenFull` has data. - **`onPickPlant`** (GardenEditorPage.tsx:251) no longer has the silent early-return on missing plant — every branch now reaches `setPicker(null)` and either arms or mutates. The `canEdit` guard remains as defense-in-depth. - **`add`** (seedTray.ts:60) handles the already-present case idempotently (`prev.includes(plant.id)` → returns same ref, no spurious save) and caps growth with `slice(-TRAY_MAX)`. - **`useSeedTray` effect** (seedTray.ts:48) correctly resets tray ids when the route `gardenId` changes without a remount; the lazy `useState` initializer + effect double-load on mount is harmless. - **`SeedTray`** renders fine with an empty `plants` array (just shows the trailing + chip) and with `armedPlantId === null` (no active highlight). No null-deref on the map. - No new subscriptions, timers, or resources introduced, so no missing `defer`/cleanup concerns. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** Reviewed through the 🔒 Security lens: - **LocalStorage data handling (`seedTray.ts`, `PlantPicker.tsx`):** Parsed data is guarded by `Array.isArray()` and `typeof n === 'number'` filters. Even if an attacker tampers with `localStorage` manually, prototype pollution or type confusion is not possible—invalid shapes fall back to `[]`. No secrets or credentials are stored. - **Dynamic content rendering (`SeedTray.tsx`, `PlantIcon.tsx`):** `p.name` and `p.icon` are rendered via JSX expression braces (`{…}`), so React HTML-escapes them automatically. No `dangerouslySetInnerHTML` is introduced. - **CSS injection (`PlantIcon.tsx`):** The `color` prop is passed through React's `style` object (not a raw CSS string), so browsers parse it as a single `backgroundColor` value; breaking out to inject additional declarations is not possible. - **Authz gating (`GardenEditorPage.tsx`):** The `canEdit` check and early-return in `onPickPlant` remain intact. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I verified the bug fix and new feature logic across all changed files: - **Bug fix** (`GardenEditorPage.tsx:251`): `onPickPlant` now receives the full `Plant` directly from the picker instead of re-resolving against `plantsById` (which only contains already-referenced plants). This correctly eliminates the early-return path that left the picker open and nothing armed. Verified by reading `PlantPicker.tsx:44` to confirm `onSelect` passes `(plant: Plant)`. - **Cache hydration** (`objects.ts:88-97`): `useEnsurePlantInFull` patches the `/full` query cache to splice a newly-chosen catalog plant into the `plants` list before any plop renders. Verified that `patchFullCache` (`objects.ts:345`) uses `qc.setQueryData`, which notifies `useGardenFull` subscribers so `plantsById` rebuilds on the next render. This prevents a missing-icon/color flash. - **Seed Tray persistence** (`seedTray.ts`): Only IDs are stored; they resolve against the live `usePlants` catalog on read. A deleted plant drops out (`.filter((p): p is Plant => !!p)`), and a renamed plant shows current data. The `TRAY_MAX = 24` cap with `.slice(-TRAY_MAX)` correctly preserves the most recent 24. `loadIds` safely handles missing keys, invalid JSON, non-arrays, and non-number entries. - **Placement pipeline** (`GardenEditorPage.tsx:242-245`, `GardenCanvas.tsx:141-151`): `armPlant` calls `ensurePlant` before `setArmedPlant`. When the user taps the bed, `createPlanting.mutate` uses `armedPlant.id` directly, and `useCreatePlanting`'s `onSuccess` splices the new plop into cache. The plant is already in the cache's `plants` array from `ensurePlant`, so `PlopLayer`'s `plantsById.get(p.plantId)` lookup succeeds immediately. - **Change-mode picker** (`GardenEditorPage.tsx:254-256`): In `'change'` mode, `ensurePlant` is called before `updatePlanting.mutate`, so the optimistic patch in `useUpdatePlanting` (`objects.ts:253-256`) renders the new plant's icon/color immediately in `PlopInspector`. Everything re-derives correctly from first principles. No logic bugs or semantic errors found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found The PR changes are clean from a performance perspective: - `useSeedTray` caps the tray at `TRAY_MAX = 24` (`web/src/lib/seedTray.ts:14`), so the map/filter/resolve work is bounded and trivial. - `useEnsurePlantInFull` (`web/src/lib/objects.ts:88`) patches the React Query cache with an immutable update on user interaction (arming/placing a plant), not inside any loop or hot path. Returning the same `full` reference when the plant is already present avoids unnecessary cache churn. - The `trayPlants` resolution (`web/src/lib/seedTray.ts:55`) is O(TRAY_MAX) and only recomputes when `ids` or the plant catalog changes. - No new query waterfalls or N+1 patterns are introduced; `usePlants` (global catalog) and `useGardenFull` (per-garden) were already both in use on this page. Nothing in this diff introduces an efficiency regression worth flagging. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Verdict: Minor issues** - **`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. - **`web/src/lib/objects.ts:92`** — `useEnsurePlantInFull` silently no-ops when the `/full` cache has no data. `patchFullCache` returns `undefined` untouched if the cache entry is missing (e.g., during a race with cache eviction or invalidation), so the caller proceeds to arm the plant without its metadata ever being spliced in. The helper should return a success flag or warn so the caller can decide whether to proceed with arming. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> The draft review confirms the security findings accurately. localStorage parsing is safely wrapped, plants are zod-validated, and gating logic is preserved. The draft correctly concludes no material issues. No material issues found - Verified `seedTray.ts`: `loadIds` (lines 16-23) wraps `JSON.parse` in try/catch and filters with `Array.isArray` + `typeof n === 'number'` before use; only numeric ids are persisted and later resolved against the zod-parsed catalog (`usePlants` → `plantSchema.parse` at `plants.ts:64`). No tainted-string or object injection path. - Verified `objects.ts` `useEnsurePlantInFull` (lines 81-93): splices a `Plant` (already validated by `plantSchema` via `/plants`) into the react-query cache; no untrusted object shape is inserted. - Verified `GardenEditorPage.tsx`: `canEdit` gating preserved (`if (!canEdit) return` in `onPickPlant`); SeedTray only rendered under `canEdit && focusedObject.plantable`; `armPlant`/`onPickPlant` only reachable from those gated surfaces. - Verified `Plant` objects handed to `onSelect`/`onArm`/`onPickPlant` originate from `PlantPicker`'s `usePlants()` (zod-parsed server data), not user-controlled strings. Plant names render via React escaping (title/aria-label/text) — no XSS vector. - Verified `gardenId` in the localStorage key comes from a typed route param (`Number(gardenId)`); no path/key injection. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> Reading the actual files to verify the change against the checked-out code rather than the diff alone. **VERDICT: No material issues found** I verified the following through the codebase: - `objects.ts:88-97` — `useEnsurePlantInFull` calls `patchFullCache` (defined at `objects.ts:345-347`), which uses `qc.setQueryData` with the updater returning `full ? fn(full) : full`. The `.some()` guard makes it a no-op when the plant already exists. The spliced plant matches `plantSchema` (the same schema the /full payload's `plants` array is validated against at `objects.ts:38-44`), so `plantsById` (rebuilt from `full.data?.plants` at `GardenEditorPage.tsx:64-65`) will resolve it. Confirmed `GardenCanvas.tsx` / `PlopLayer.tsx` render plops via `plantsById.get(p.plantId)`, so the fix actually makes the new plant's icon/color appear. ✓ - `GardenEditorPage.tsx:251-261` — `onPickPlant` now uses the `Plant` handed back by `PlantPicker.choose` (`PlantPicker.tsx:76-79`, which calls `onSelect(p)` with the full catalog plant). The early `return` on a missing plant is gone, so `setArmedPlant` / `updatePlanting.mutate` and `setPicker(null)` all run. This is exactly the described root-cause fix. ✓ - `seedTray.ts:60-69` — `add` dedups by id and `.slice(-TRAY_MAX)` keeps the most-recent 24 (drops oldest when over the cap). Re-derived: with `prev` length 24 and one append → length 25 → `slice(-24)` keeps indices 1..24, correctly evicting the oldest. ✓ - `seedTray.ts:48-50` — the `useEffect` resets `ids` on `gardenId` change so route-param switches (without remount) re-read the per-garden localStorage key. `add`/`remove` capture `gardenId` in deps and call `saveIds(gardenId, …)`, so they write to the correct key. ✓ - `seedTray.ts:55-58` — `trayPlants` filters out ids not in the live catalog (`byId`), so a deleted plant silently drops out as documented. ✓ - Ordering in `'change'` mode (`GardenEditorPage.tsx:254-256`): `ensurePlant(plant)` runs synchronously (`setQueryData`), then `updatePlanting.mutate`'s `onMutate` patches the planting's `plantId` synchronously (`objects.ts:250-257`). By the next render both the plant is in `full.plants` and the plop's `plantId` is updated, so `PlopInspector` (`GardenEditorPage.tsx:343` reads `plantsById.get(selectedPlop.plantId)`) shows the new plant's icon immediately. ✓ - `addToTray(plant)` is called unconditionally in `onPickPlant` (both 'place' and 'change' modes). This matches the PR's stated feature behavior ("Picking from the catalog → adds to the tray") and isn't gated on anything that could fail silently. ✓ The "Done" button no longer displays `armedPlant.icon` (it now just says "Done"), and the empty-state hint says "Pick a plant from the tray" even when the tray is empty on a fresh garden — but both are UX/copy nits, not correctness bugs in my lane. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## VERDICT: Minor issues - **`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 documenting this in the comment or only adding to the tray in the `'place'` branch (which is the actual "kept at hand" use case the tray is for). - **`web/src/pages/GardenEditorPage.tsx:242-261`** — `ensurePlant(plant)` is invoked in `armPlant` (line 243) and *again* inline in the `'change'` branch of `onPickPlant` (line 255), while the `'place'` branch goes through `armPlant`. The branching mixes two call styles for the same pre-step. Hoisting a single `ensurePlant(plant)` to the top of `onPickPlant` (it's a no-op when already present) would remove the duplication and let `armPlant` focus on `setArmedPlant`. Verified `useEnsurePlantInFull` is idempotent (early-return `some(...)` check at `objects.ts:93`). - **`web/src/editor/SeedTray.tsx:62`** — The trailing chip's label uses the full-width `+` (U+FF0B), whereas the rest of the codebase uses a regular ASCII `+` (e.g., the just-removed `+ Add plant` button, and `+ Add plant` patterns elsewhere). Cosmetic, but it's a stray character-class inconsistency that wasn't present before. - **`web/src/lib/seedTray.ts:36-37 / 64`** — The `add` JSDoc says "Add a plant to the end of the tray (no-op if already present)" but doesn't mention the silent eviction when full: `.slice(-TRAY_MAX)` drops the *oldest* entries. `TRAY_MAX = 24` is a working-set cap, and the eviction policy (FIFO-by-age) is a real behavior a maintainer would want to know without reading the implementation. Suggest adding "(evicting the oldest past `TRAY_MAX`)" or similar to the doc. - **`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. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found Through the performance lens, I verified the new code paths and found no material regressions: - `web/src/lib/seedTray.ts:52-58` — `byId` Map and `trayPlants` resolution are both `useMemo`-gated on `[plants.data]` and `[ids, byId]` respectively, so the per-render work is bounded and only recomputes when inputs actually change. Tray size is capped at `TRAY_MAX = 24` (seedTray.ts:14), so `.includes()` / `.filter()` / `.map()` on the id list are all O(≤24) — not hot. - `web/src/lib/objects.ts:88-97` (`useEnsurePlantInFull`) — `full.plants.some(...)` is O(referenced plants) and only fires from event handlers (`armPlant`/`onPickPlant`), not on render. The cache splice copies the plants array once per arm; negligible. - `web/src/pages/GardenEditorPage.tsx` — `armPlant`/`onPickPlant` are event handlers, not render-path work; `ensurePlant` is called at most once per pick (the `'change'` and `'place'` branches are mutually exclusive). - `useEffect` re-read of `loadIds` on `gardenId` change (seedTray.ts:48-50) is a single synchronous localStorage read on route change, not a hot path. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I read `objects.ts`, `seedTray.ts`, `plants.ts`, and `GardenEditorPage.tsx` to verify the unhappy paths. **VERDICT: No material issues found** Through the error-handling & edge-case lens, the change is clean: - `loadIds`/`saveIds` (seedTray.ts:16-31) both wrap `localStorage` in `try/catch` and degrade gracefully (return `[]` / no-op). The swallowed quota/availability failures are explicit and documented in the file header — intentional, not a bug. - `loadIds` (seedTray.ts:19) defensively validates storage shape: `Array.isArray(v) && typeof n === 'number'`. `JSON.parse` can't smuggle `NaN`/`Infinity` through (they're not valid JSON), so the filter is sufficient. Verified. - `ensurePlant` → `patchFullCache` (objects.ts:345-347) is a no-op when the `/full` cache is absent (`full ? fn(full) : full`), so calling it before the query resolves can't crash. The page early-returns on `full.isPending` anyway. Verified. - `useSeedTray`'s `useEffect` (seedTray.ts:48-50) re-reads on `gardenId` change. No data loss: `saveIds` always writes through the current `gardenId` closure, so the old garden's tray is already persisted before the effect overwrites in-memory `ids`. - `onPickPlant` 'change' branch (GardenEditorPage.tsx:254-256): a failed `updatePlanting.mutate` is handled by the mutation's existing `onError` (objects.ts:269-281), which rolls back and toasts. The optimistic `ensurePlant(plant)` leaves a now-unreferenced plant in the `/full` cache's `plants` list on failure, but that self-heals on the next refetch (`ListReferencedPlants` won't include it). Transient, not material. - `onPickPlant` else-branch (GardenEditorPage.tsx:257-258) also covers the `picker === 'change' && !selectedPlop` edge — it arms the plant rather than doing nothing. Slightly odd UX, but not an unhandled error path. - Empty tray, empty catalog (`plants.data` undefined → `byId` empty → `trayPlants` empty), and tray-full eviction (`slice(-TRAY_MAX)`) all degrade without crashing. Verified against SeedTray.tsx render (always renders the trailing + chip regardless of `plants.length`). One non-material note (not rising to a finding): `saveIds` is invoked inside the `setIds` updater in `add`/`remove` (seedTray.ts:65, 75) — a side effect inside a state updater, which React expects to be pure. It's idempotent (same value written) so StrictMode double-invocation is harmless, and I confirmed no concrete corruption path, so I'm not flagging it as a finding. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found. - **localStorage deserialization is safely handled.** `loadIds` in `web/src/lib/seedTray.ts:16` parses from `localStorage`, wraps in `try/catch`, and immediately filters the result to an array of numbers. This prevents prototype-pollution or type-confusion from malicious stored JSON. - **No XSS via `Plant.name` or `Plant.icon`.** In `web/src/editor/SeedTray.tsx`, `p.name` is interpolated into `title`, `aria-label`, and text content via standard JSX expressions (`{p.name}`), which React escapes. `PlantIcon` renders `icon` as a text child, not via `dangerouslySetInnerHTML`. - **No authz bypass.** The `canEdit` gate at `GardenEditorPage.tsx:252` remains in place before any plant is armed or mutated. Relaxing the client-side resolution from `plantsById` to the full catalog simply fixes a bug where the UI was *over*-restrictive; the server remains the authoritative boundary for which plants can actually be placed. - **Cache injection in `useEnsurePlantInFull` is benign.** The injected `Plant` originates from the already-trusted `usePlants()` catalog (same source as the `PlantPicker`), and the cache is client-only UI state that will be overwritten on the next refetch. - **No secret leakage, command injection, SSRF, or insecure defaults** are introduced. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I reviewed the PR through the **Correctness** lens, tracing the logic across all four changed files plus dependencies (`PlantPicker`, `GardenCanvas`, `PlopInspector`, `usePlants`). The code is sound: * **Bug fix verified:** The old `onPickPlant(plantId)` looked up the plant in `plantsById` (built from referenced plants only), causing an early `return` for never-before-used plants. The new `onPickPlant(plant)` receives the full `Plant` from `PlantPicker` directly and uses it. This matches the root-cause analysis exactly. * **Cache splicing verified:** `useEnsurePlantInFull` patches the `garden-full` cache's `plants` array when a not-yet-referenced plant is armed/placed. I confirmed `patchFullCache` uses `qc.setQueryData`, which notifies `useGardenFull` subscribers, so `plantsById` recomputes before any plop renders. * **Change-mode path verified:** In `'change'` mode, `ensurePlant(plant)` is called before `updatePlanting.mutate(...)`, ensuring the cache contains the new plant for optimistic rendering. The `onMutate` of `useUpdatePlanting` only patches `plantings`, leaving the `plants` array intact, so the pre-inserted plant remains available. * **Seed Tray logic verified:** `useSeedTray` resolves stored ids against the live catalog (`usePlants`), drops deleted plants, caps at `TRAY_MAX = 24`, and re-reads on `gardenId` change. The `add`/`remove` callbacks use functional `setIds` with synchronous `localStorage` writes, which is safe for the intended convenience-store semantics. * **No stale closures or missed edges:** `armPlant` correctly calls `ensurePlant` then `setArmedPlant`. The `onPlace` handler in `GardenCanvas` fires after arming, by which time the plant is already in the cache. No callers set `armedPlant` without ensuring the cache is primed. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`web/src/pages/GardenEditorPage.tsx:358`** — `onSelect={(p) => onPickPlant(p)}` is an unnecessary wrapper now that `onPickPlant` accepts `(plant: Plant) => void`, matching `PlantPicker`'s `onSelect` signature exactly. Pass `onSelect={onPickPlant}` directly to avoid creating a new arrow on every render and to keep the call site cleaner. - **`web/src/lib/seedTray.ts:33`** — The exported interface `SeedTray` collides with the `SeedTray` React component in `web/src/editor/SeedTray.tsx`. This is confusing for readers and can trip up auto-imports / "Go to definition" in editors. Rename the interface to `UseSeedTrayResult` or `SeedTrayState`. - **`web/src/pages/GardenEditorPage.tsx:287-310`** — The JSX guard `canEdit && focusedObject.plantable` is repeated across three separate expressions. Consolidating the plantable branch into a single wrapper (`{canEdit && focusedObject.plantable && (<>{...}</>)}`) makes the focus-toolbar logic easier to maintain and less prone to partial updates if the guard rules change. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** Through the **Performance** lens, the PR is clean. I verified the changed files and related call sites (`GardenCanvas`, `PlantPicker`, `plants.ts`) and found no efficiency regressions: - **`useSeedTray`** (`web/src/lib/seedTray.ts:43`) keeps the tray capped at 24 items (`TRAY_MAX`), so the `ids→plants` resolution via `useMemo` is bounded O(1) work. The `Map` rebuild from the full catalog on `plants.data` changes is unavoidable for live resolution and is protected by React Query’s structural-sharing reference stability. - **`useEnsurePlantInFull`** (`web/src/lib/objects.ts:88`) runs an O(n) `.some()` scan over referenced plants only on user interaction (picker select / tray arm), never inside a render loop or animation frame. The cache patch is a single immutable splice, not a repeated churn. - **No new unbounded work in hot paths.** The `SeedTray` render maps over at most 24 chips; `armPlant` and `onPickPlant` are event handlers, not per-frame operations. The canvas `onPlace` handler is untouched by this diff and remains pointer-event bound. - **Network behavior** is acceptable: `useSeedTray` pulls in `usePlants()`, which means the full catalog now loads on editor mount rather than lazily when the picker opens. This is a necessary trade-off for live ID→plant resolution (as documented in the PR), and React Query will serve it from cache after the first fetch. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## Review: Error handling & edge cases **Verdict:** Minor issues ### Findings - **`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:** Cap the array in `loadIds` before returning: ```ts const arr = Array.isArray(v) ? v.filter((n): n is number => typeof n === 'number') : [] return arr.slice(-TRAY_MAX) ``` **Verified** by reading `seedTray.ts` and confirming `TRAY_MAX` is only used in the `add` updater. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-19 06:08:56 +00:00
Address review: disarm on tray-remove, cap load, tidy
Build image / build-and-push (push) Successful in 9s
30e8d3e56c
- Removing the armed plant from the tray now disarms it, so placement
  doesn't continue for a plant that's no longer shown.
- loadIds honors TRAY_MAX even if localStorage was hand-edited.
- Consolidate the repeated `canEdit && plantable` toolbar guards into one
  nested conditional (3-model finding).
- Rename the hook's SeedTray interface to SeedTrayState (avoids colliding
  with the SeedTray component) and the component prop plants → trayPlants.
- Hoist ensurePlant to one call in onPickPlant; pass onPickPlant directly as
  onSelect; use ASCII "+" for the tray chip; document the FIFO eviction.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit 5cdc2779d7 into main 2026-07-19 06:09:51 +00:00
steve deleted branch fix/plant-placement-and-seed-tray 2026-07-19 06:09:51 +00:00
Sign in to join this conversation.