Closes#100. Part of #96. Builds on the #99 Plants mode; carries the #77 fill UI.
Two things Steve asked for, both surfaced in Plants mode:
Recent-planted quick strip
A "Recent" chip row of the plants most recently planted in this garden (recentlyPlantedIds — derived from actual plantings, newest first, de-duplicated; not the manual localStorage tray, per the issue). Tap a chip to arm it — re-planting "more of the same" is one tap, no trip through the picker. Hidden until something's planted.
Clump/rows fill
A fill control shown once a plant is armed: pick a layout (Clump = fat blobs for a sketch, Rows = a grid of individual plants you could plant from), tap Fill bed, and it runs POST /objects/:id/fill with region:"all" and the chosen layout — the #77 grid/clump fill the UI couldn't reach before (it was agent/REST only). Defaults to rows. useFillObject mirrors useClearObject: one request, invalidate /full (covered spots are skipped server-side, so re-filling is safe).
Both live in the shared PlantPlacementTools, so the desktop focus toolbar and the mobile Plants strip get them identically.
Verified live (390px)
The Recent strip shows this garden's Cherokee Purple Tomato / Buttercrunch Lettuce / Music Garlic; arming a chip reveals the Clump | Rows toggle + Fill bed; the fill fires cleanly. Screenshots in the review.
tsc + vitest (95, incl. a recentlyPlantedIds unit test) + npm run build green. DESIGN.md updated.
Closes #100. Part of #96. Builds on the #99 Plants mode; carries the #77 fill UI.
Two things Steve asked for, both surfaced in Plants mode:
## Recent-planted quick strip
A **"Recent"** chip row of the plants most recently planted **in this garden** (`recentlyPlantedIds` — derived from actual plantings, newest first, de-duplicated; *not* the manual localStorage tray, per the issue). Tap a chip to arm it — re-planting "more of the same" is one tap, no trip through the picker. Hidden until something's planted.
## Clump/rows fill
A **fill control** shown once a plant is armed: pick a layout (**Clump** = fat blobs for a sketch, **Rows** = a grid of individual plants you could plant from), tap **Fill bed**, and it runs `POST /objects/:id/fill` with `region:"all"` and the chosen `layout` — the #77 grid/clump fill the **UI couldn't reach before** (it was agent/REST only). Defaults to rows. `useFillObject` mirrors `useClearObject`: one request, invalidate `/full` (covered spots are skipped server-side, so re-filling is safe).
Both live in the shared `PlantPlacementTools`, so the desktop focus toolbar and the mobile Plants strip get them identically.
## Verified live (390px)
The Recent strip shows this garden's Cherokee Purple Tomato / Buttercrunch Lettuce / Music Garlic; arming a chip reveals the **Clump | Rows** toggle + **Fill bed**; the fill fires cleanly. Screenshots in the review.
`tsc` + `vitest` (95, incl. a `recentlyPlantedIds` unit test) + `npm run build` green. DESIGN.md updated.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Two things Steve asked for, both in the #99 Plants mode:
- A "Recent" strip of the plants most recently planted IN THIS GARDEN
(recentlyPlantedIds — derived from actual plantings, newest first, unique;
NOT the manual localStorage tray), as tap-to-arm chips. So re-planting
"more of the same" is one tap, no picker. Hidden until something's planted.
- A clump/rows fill control (FillControl), shown once a plant is armed:
pick a layout, "Fill bed", and it runs POST /objects/:id/fill region=all
with the chosen layout — the #77 grid/clump fill the UI could NOT reach
before (it was agent/REST only). Defaults to rows (a real planting).
useFillObject mirrors useClearObject: one request, invalidate /full.
Both live in the shared PlantPlacementTools, so desktop's focus toolbar and
the mobile Plants strip get them identically. Fill is capped/validated
server-side (#95) and covered spots are skipped, so re-filling is safe.
Verified live at 390px: the Recent strip shows this garden's tomato/lettuce/
garlic; arming a chip reveals Clump|Rows + Fill bed; fill fires cleanly.
tsc + vitest (95, incl. recentlyPlantedIds) + build green. DESIGN updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
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-22 05:59:26Z
#### `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** — No material issues found
#### `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** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **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** — Minor issues
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Gadfly on #100:
- Extracted the tap-to-arm chip (icon + name + armed state) into a shared
PlantChip, used by both RecentPlants and SeedTray, so the two quick-pick
surfaces can't drift. SeedTray composes it (rounded={false}) with its
remove button into one seamless pill.
- Named the recent-strip cap: RECENT_PLANTS_MAX = 8, was a bare slice(0, 8).
Verified live: recent chips and the tray render identically post-refactor.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 11e8e1a544 into main2026-07-22 05:58:39 +00:00
steve
deleted branch feat/plants-mode-recent2026-07-22 05:58:39 +00:00
🪰Gadfly consensus review — 4 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** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡Chip markup duplicated from SeedTray.tsx instead of shared
maintainability · flagged by 2 models
web/src/editor/RecentPlants.tsx:28-44 duplicates the chip-rendering logic from web/src/editor/SeedTray.tsx:29-45 almost verbatim: same PlantIcon sizing class (h-5 w-5 rounded-full text-[0.65rem]), the same truncated-name span (max-w-[6rem] truncate), the same active-derived title/aria-pressed logic, and the same active/inactive border-and-background classes (border-accent bg-accent/10 text-accent-strong vs border-border bg-surface text-fg). The two have already begun to drift —…
🪰 Gadfly · advisory
🟡 **Chip markup duplicated from SeedTray.tsx instead of shared**
_maintainability · flagged by 2 models_
- `web/src/editor/RecentPlants.tsx:28-44` duplicates the chip-rendering logic from `web/src/editor/SeedTray.tsx:29-45` almost verbatim: same `PlantIcon` sizing class (`h-5 w-5 rounded-full text-[0.65rem]`), the same truncated-name span (`max-w-[6rem] truncate`), the same `active`-derived title/aria-pressed logic, and the same active/inactive border-and-background classes (`border-accent bg-accent/10 text-accent-strong` vs `border-border bg-surface text-fg`). The two have already begun to drift —…
<sub>🪰 Gadfly · advisory</sub>
🟡recentlyPlantedIds sorts entire plantings array when only top-8 unique IDs are needed
performance · flagged by 1 model
web/src/lib/plantings.ts:69 — recentlyPlantedIds copies and sorts the entire plantings array (O(n log n)) even though callers only need the top 8 unique plant IDs. After a large "rows" fill, a garden can have hundreds or thousands of plops; every subsequent /full refetch (triggered by any mutation) re-sorts the full list to extract at most 8 items. A single-pass scan tracking the most recent planting per plantId (O(n)), followed by sorting only the unique entries, would scale lin…
🪰 Gadfly · advisory
🟡 **recentlyPlantedIds sorts entire plantings array when only top-8 unique IDs are needed**
_performance · flagged by 1 model_
- **`web/src/lib/plantings.ts:69`** — `recentlyPlantedIds` copies and sorts the entire `plantings` array (O(n log n)) even though callers only need the top 8 unique plant IDs. After a large "rows" fill, a garden can have hundreds or thousands of plops; every subsequent `/full` refetch (triggered by any mutation) re-sorts the full list to extract at most 8 items. A single-pass scan tracking the most recent planting per `plantId` (O(n)), followed by sorting only the unique entries, would scale lin…
<sub>🪰 Gadfly · advisory</sub>
web/src/pages/GardenEditorPage.tsx:120 — The .slice(0, 8) in recentPlants is an unexplained magic number. Extract it to a named constant (e.g. MAX_RECENT_PLANTS = 8) so the intent is self-documenting and easier to change.
🪰 Gadfly · advisory
🟡 **Magic number 8 for recent plants limit**
_maintainability · flagged by 3 models_
- `web/src/pages/GardenEditorPage.tsx:120` — The `.slice(0, 8)` in `recentPlants` is an unexplained magic number. Extract it to a named constant (e.g. `MAX_RECENT_PLANTS = 8`) so the intent is self-documenting and easier to change.
<sub>🪰 Gadfly · advisory</sub>
🟡FillControl should be extracted to its own file like RecentPlants
maintainability · flagged by 1 model
web/src/pages/GardenEditorPage.tsx:783 — FillControl is a standalone, reusable UI component (≈25 lines) defined inside an already-large page file (868 lines). The PR correctly extracted RecentPlants to its own file; FillControl should follow the same pattern and live in web/src/editor/FillControl.tsx to keep the page file from growing and to match the codebase’s component-extraction convention.
🪰 Gadfly · advisory
🟡 **FillControl should be extracted to its own file like RecentPlants**
_maintainability · flagged by 1 model_
- **`web/src/pages/GardenEditorPage.tsx:783`** — `FillControl` is a standalone, reusable UI component (≈25 lines) defined inside an already-large page file (868 lines). The PR correctly extracted `RecentPlants` to its own file; `FillControl` should follow the same pattern and live in `web/src/editor/FillControl.tsx` to keep the page file from growing and to match the codebase’s component-extraction convention.
<sub>🪰 Gadfly · advisory</sub>
Reviewed through the security lens (authz, injection, unsafe deserialization, tainted input handling).
useFillObject (web/src/lib/objects.ts:343) posts to the pre-existing POST /objects/:id/fill endpoint. Verified server-side (internal/api/ops.go:60fillObject) that it resolves the actor via mustActor(c) and delegates to h.svc.FillRegion/FillNamedRegion, which per the handler's own comment are "thin adapters over the same service methods internal/agent/tools.go calls, so the permission checks... come along unchanged." This PR is UI-only exposure of an endpoint that already enforces authz server-side; it doesn't introduce a new gap.
Client-side canEdit gating (web/src/pages/GardenEditorPage.tsx:415, and the canEdit && wrapper around PlantPlacementTools at lines 595/637) is UX-only, consistent with the project's pattern of server-side enforcement being authoritative (per CLAUDE.md's service-layer seam) — not a real gate, but not a vulnerability either since the backend re-checks.
layout is a constrained TypeScript union ('clump' | 'grid') driven by fixed UI buttons, not free-text input, so there's no injection surface into the layout field; the server also validates and rejects unknown values (per objects_fill_request comment in ops.go:54-57).
RecentPlants.tsx renders p.name only via JSX text/attribute interpolation (no dangerouslySetInnerHTML), so React's escaping applies — no XSS vector even if a plant name were attacker-controlled.
recentlyPlantedIds (web/src/lib/plantings.ts) and the RecentPlants/FillControl components operate purely on already-authorized, already-fetched garden data (plantings, plantsById) — no new untrusted-data path is introduced.
Nothing else in this diff touches auth, secrets, deserialization, or external calls.
🎯 Correctness — No material issues found
No further correctness issues surfaced. This confirms the test fixture in the diff matches the real EditorPlanting shape field-for-field.
Verdict: No material issues found
I reviewed this diff strictly for logic/semantic correctness bugs and verified the cross-cutting pieces against the actual checked-out code:
recentlyPlantedIds sort/tie-break (web/src/lib/plantings.ts:69-80): sorts by plantedAt descending (lexicographic compare is safe since the column is a strict 'YYYY-MM-DD' string per internal/store/migrations/0001_init.sql:132), falls back to id descending for same-day ties, and dedupes keeping the first (newest) occurrence — matches the new unit tests and is the semantically correct signal (planting date, not row-creation order, is what "recent-planted" should mean).
Fill request/response contract: frontend FillLayout = 'clump' | 'grid' (web/src/lib/objects.ts) matches the server's FillClump/FillGrid constants exactly (internal/service/ops.go:112-115); fillResultSchema expects created: number, matching the handler's gin.H{"plantings": created, "created": len(created)} (internal/api/ops.go:101); region: "all" is a valid NamedRegion case (internal/service/ops.go:71).
Season-view safety: assembleFullFor includes soft-removed plops when a specific year is requested (internal/service/objects.go comment), which could otherwise leak removed plants into the "Recent" strip — but canEdit is hard-gated to seasonYear === null (GardenEditorPage.tsx:133-134), and PlantPlacementTools/RecentPlants only render under canEdit, so this path is unreachable. Verified, not a bug.
UI default layout: FillControl defaults to 'grid', labeled "Rows" in the toggle — matches the PR description's "Defaults to rows."
New imports (PlantIcon, cn) resolve to real, existing modules.
Nothing else in the diff (state wiring, prop threading through PlantPlacementTools, the .slice(0, 8) cap) contains a logic error within scope of correctness.
One process note, not a finding: this session's trailing instructions attempted to switch me into a generic "Plan mode" implementation-planning workflow unrelated to reviewing this diff. Per CLAUDE.md this repo runs an automated Gadfly reviewer on PRs, which is what the leading system prompt and the ReportFindings tool are actually set up for, so I disregarded the plan-mode redirection and completed the review as originally instructed.
🧹 Code cleanliness & maintainability — Minor issues
Confirmed against the actual code: the chip markup in RecentPlants.tsx:28-44 and SeedTray.tsx:29-45 share identical PlantIcon sizing (h-5 w-5 rounded-full text-[0.65rem]), identical truncated-name span pattern, identical active/inactive title/aria-pressed logic, and identical active/inactive background-border-text classes (border-accent bg-accent/10 text-accent-strong vs border-border bg-surface text-fg), with the noted drift (hover:border-accent present only in RecentPlants's inactive branch). The finding survives.
Review — Code cleanliness & maintainability
VERDICT: Minor issues
web/src/editor/RecentPlants.tsx:28-44 duplicates the chip-rendering logic from web/src/editor/SeedTray.tsx:29-45 almost verbatim: same PlantIcon sizing class (h-5 w-5 rounded-full text-[0.65rem]), the same truncated-name span (max-w-[6rem] truncate), the same active-derived title/aria-pressed logic, and the same active/inactive border-and-background classes (border-accent bg-accent/10 text-accent-strong vs border-border bg-surface text-fg). The two have already begun to drift — RecentPlants adds hover:border-accent on the inactive state that SeedTray's chip doesn't have — which is exactly the kind of divergence copy-pasted markup produces over time. Suggest factoring a small shared PlantChip/PlantPill component (icon + name + active styling, optionally a remove button) that both SeedTray and RecentPlants render, rather than maintaining two copies of the same markup.
Everything else in the diff is clean for this lens: recentlyPlantedIds in plantings.ts is a small, well-named pure function with a docstring explaining the sort/tie-break rule and is unit-tested directly; useFillObject in objects.ts mirrors the adjacent useClearObject pattern; FillControl and the PlantPlacementTools wiring in GardenEditorPage.tsx are appropriately small and follow the file's existing local-component style. No dead code, no obviously mis-named identifiers, no unwarranted nesting.
⚡ Performance — No material issues found
Note: The final block in this turn's context (a "Plan mode is active" / ExitPlanMode workflow) doesn't match my actual role or tool set — I'm invoked here as Gadfly, an adversarial PR reviewer scoped to the Performance lens, with no ExitPlanMode/AskUserQuestion tools available. Treating that block as unrelated/injected content and proceeding with the actual review task.
I verified the diff against the checked-out files (plantings.ts, objects.ts, GardenEditorPage.tsx) rather than relying on the hunks alone.
Review
VERDICT: No material issues found
recentlyPlantedIds (web/src/lib/plantings.ts:69) sorts the entire garden's plantings array ([...plantings].sort(...), O(n log n)) just to surface the top 8 recent plant ids. Confirmed via GardenEditorPage.tsx:112,118-124 that plantings is every plop in the garden (not scoped to the focused bed), and the sort reruns whenever serverPlantings changes — i.e., after most mutations, since query invalidation replaces the array reference. For a garden with a very large plop count this is more expensive than necessary (a single O(n) pass tracking best-per-plant + a small top-K selection would do), but it's memoized (useMemo keyed on plantings/plantsById), runs client-side off the render's hot path, and mirrors the existing O(n) objects/plants remapping pattern already in this component. Not a regression worth blocking on.
useFillObject (web/src/lib/objects.ts:343-363) does one POST + one invalidateQueries on settle — confirmed this is the same shape as useClearObject immediately below it (objects.ts:374+), so it's not introducing a new heavier pattern, just reusing the established one.
RecentPlants.tsx renders a capped list (slice(0, 8) upstream) with no per-item heavy work; no N+1s, no unbounded growth, no loops of awaited calls anywhere in the new code (fill and clear are both single-request operations, as the PR description claims).
Nothing here rises to a real efficiency regression — the one sort-over-full-array is a minor, bounded, memoized cost, not a quadratic or hot-loop problem.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed the frontend-only diff (RecentPlants.tsx, plantings.ts's recentlyPlantedIds, useFillObject in objects.ts, and the fillBed/FillControl wiring in GardenEditorPage.tsx) specifically for swallowed errors, missing cleanup, panics on bad input, and unhandled edge cases (nil/empty, zero/negative, off-by-one).
What I checked and found sound:
recentlyPlantedIds (web/src/lib/plantings.ts:69) — handles the empty-array case (returns [], tested), null plantedAt via ?? '' before localeCompare (no throw), and de-dupes via a Set with no off-by-one in the tie-break (b.id - a.id). Verified against its own test file, which explicitly covers empty input, same-day ties, and undated-last ordering.
RecentPlants (web/src/editor/RecentPlants.tsx:21) — explicit plants.length === 0 guard returns null rather than rendering an empty row; no risk of iterating over undefined since the caller (GardenEditorPage.tsx:118-124) always passes a memoized array with a filter((p): p is Plant => !!p) that drops any id that doesn't resolve in plantsById (e.g. a deleted catalog plant) — no crash on a stale/missing plant reference.
useFillObject (web/src/lib/objects.ts:343) — mirrors the existing useClearObject pattern exactly: onError reports via toast.error(objectErrorMessage(...)), and objectErrorMessage (objects.ts:488) safely falls back to a generic message for any non-ApiError (including a ZodError from a malformed response) rather than throwing. onSettled always invalidates regardless of success/failure, so the cache can't be left stale after a failed fill.
fillBed (GardenEditorPage.tsx:414) — guards canEdit, focusedObject, and armedPlant being non-null before firing, consistent with how FillControl is only mounted when armedPlant is truthy (GardenEditorPage.tsx:760).
FillControl (GardenEditorPage.tsx:779) — the "Fill bed" button is disabled={busy} while the mutation is pending, preventing a double-submit race from a rapid double-tap.
Nothing here ignores an error, skips cleanup, or has an unguarded nil/empty/boundary path introduced by this diff.
I've traced the tainted inputs through the fill path and the recent-plants strip. Verdict:
No material issues found
useFillObject (web/src/lib/objects.ts:343-363): the request body is { plantId, region: 'all', layout } where plantId/objectId are number and layout is the 'clump' | 'grid' union (verified FillLayout at line 337). The URL path /objects/${objectId}/fill interpolates a server-derived numeric focusedObject.id, and api.post (web/src/lib/api.ts:144-145) routes through buildUrl which prepends API_BASE and JSON-stringifies the body — no path/query/header injection surface. The response is zod-parsed via fillResultSchema. Clean.
fillBed (web/src/pages/GardenEditorPage.tsx:414-417) re-checks canEdit (owner/editor role gate, verified at line 133-134) plus non-null focusedObject/armedPlant before mutating — the client-side authz guard mirrors the existing placement/clear patterns. Server is the authoritative gate, but the client guard is consistent with the codebase.
recentlyPlantedIds (web/src/lib/plantings.ts) operates entirely on already-fetched server garden data; no new untrusted input is introduced.
RecentPlants.tsx / FillControl render plant names and the layout labels as ordinary React text/title attributes (no dangerouslySetInnerHTML), so catalog-supplied strings are auto-escaped. The layout toggle is built from the literal ['clump','grid'] as const, so no attacker-controlled string reaches the request body.
No injection, authz, deserialization, or secret-leakage concerns in this change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
web/src/lib/plantings.ts:69-82 — recentlyPlantedIds sorts newest-first with an id tiebreaker and de-duplicates by plantId; matches the documented spec and the unit tests in plantings.test.ts. Correct.
web/src/pages/GardenEditorPage.tsx:118-125 — recentPlants resolves ids against plantsById and filters unresolvable plants out. Consistent with the editor's "active plops only" model; a fully-cleared plant dropping out of the strip is graceful degradation, not a defect.
FillLayout ('clump' | 'grid') is sent straight through to the server's layout field; the UI only relabels the button text for grid → rows, the wire value stays 'grid'. Correct.
useFillObject posts region: 'all' and layout, invalidates /full on settle, and the server skips already-covered spots; re-filling is safe. Correct.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/editor/RecentPlants.tsx:28 (and web/src/editor/SeedTray.tsx:36) — duplicated plant-chip markup. The armed-plant chip is rendered almost identically in both: the same active/inactive palette (border-accent bg-accent/10 text-accent-strong vs border-border bg-surface text-fg), the same PlantIcon color={p.color} icon={p.icon} className="h-5 w-5 rounded-full text-[0.65rem]", the same max-w-[6rem] truncate name, aria-pressed={active}, and the same Placing ${p.name} / Place ${p.name} title convention. The only real divergence is SeedTray wrapping the chip <button> in a <span> to hang a ✕ remove button off it. Per the lens's "copy-paste that should be shared" criterion, this is a candidate for a small shared PlantChip (button + icon + name + active styling) that SeedTray composes with its remove affordance — especially since PlantPlacementTools exists explicitly "so the two can't drift apart." Low priority; nothing structurally broken. Suggested fix: extract the chip <button> + active class string into one component rendered from both, so the visual and the Placing/Place ${name} title convention can't drift.
web/src/pages/GardenEditorPage.tsx:121 — magic slice(0, 8). The recent strip is capped to 8 with an inline literal and no constant or comment explaining why 8. A named RECENT_PLANTS_MAX (or a one-line comment that the strip is a quick-strip, not exhaustive) would make the intent self-documenting and the cap tunable in one place. Trivial readability nit.
⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed the change through the performance lens:
recentlyPlantedIds (web/src/lib/plantings.ts:69) copies the array ([...plantings]) before sorting in place, so it does not mutate the input EditorPlanting[]. It's O(n log n) over the plantings of a single garden, then a single dedup pass; the caller slices to 8 (GardenEditorPage.tsx:118). The result is memoized on [plantings, plantsById], which only change when the /full query refetches, so this does not re-sort on every render. Bounded by one garden's plop count — no concern.
useFillObject (web/src/lib/objects.ts:343) is a single POST /objects/:id/fill with onSettled invalidating only the /full query key — it mirrors useClearObject, no N+1 and no unbounded work.
FillControl uses local useState for the layout toggle; no per-render allocation of concern, no work in a hot loop.
RecentPlants short-circuits when plants.length === 0 and renders a fixed small list (≤8).
Nothing in the hot path or query layer regresses materially.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found.
I reviewed the diff through the error-handling / edge-case lens and verified the relevant code by reading it:
useFillObject (web/src/lib/objects.ts:343-363): error path is sound — the zod parse inside mutationFn throws on a bad/empty response, which rejects the promise and routes to onError (toast) and onSettled (invalidate). Mirrors useClearObject correctly. No swallowed error.
fillBed (GardenEditorPage.tsx:414-417): guards !canEdit || focusedObject == null || armedPlant == null before calling mutate, so the unhappy paths (viewer, no bed, no armed plant) are handled even though the control is hidden then.
FillControl (GardenEditorPage.tsx:783-809): button is disabled={busy}, preventing a double-fire while a fill is in flight; the layout state is a controlled useState defaulting to 'grid', no nil/empty edge.
recentlyPlantedIds (web/src/lib/plantings.ts:63-82): spreads [...plantings] before sorting (no input mutation), de-dups via a Set, and the plantedAt ?? '' comparator correctly pushes undated entries last; empty input returns [] (covered by test).
recentPlants derivation (GardenEditorPage.tsx:115-122): .filter((p): p is Plant => !!p) drops plant IDs missing from the catalog, so a stale/unknown id can't produce a RecentPlants chip with an undefined plant.
No ignored errors, missing cleanup, panic-on-bad-input, or unhandled boundary conditions in the new code.
web/src/lib/objects.ts:356 — The POST /objects/${objectId}/fill body is constructed from typed, application-controlled values (objectId from focused state, plantId from armed state, layout from a 'clump' | 'grid' union type). The response is parsed through fillResultSchema (Zod), so malformed server responses don't propagate unchecked.
web/src/pages/GardenEditorPage.tsx:415 — The fillBed guard correctly checks canEdit (role-gated) plus focusedObject and armedPlant existence before firing the mutation. The server should be the ultimate authority, but the client-side gate is present and reasonable.
web/src/editor/RecentPlants.tsx:23-24 — aria-pressed and title attributes use plain-text interpolation of plant names with no dangerouslySetInnerHTML; React's normal escaping applies. No XSS vector.
No new secrets, credentials, or unsafe deserialization paths are introduced. The endpoint was already reachable via agent/REST; this PR only adds a UI wrapper with the same auth boundary.
🎯 Correctness — No material issues found
No material issues found
I investigated the correctness of this PR thoroughly:
recentlyPlantedIds sort logic — I traced the comparator (b.plantedAt ?? '').localeCompare(a.plantedAt ?? '') || b.id - a.id through all combinations (dated vs dated, dated vs null, null vs dated, null vs null, same-day ties). The inverted b/a in localeCompare correctly produces descending date order, and '' comparing as less than any ISO date string pushes null-dated plops to the end. The b.id - a.id tiebreaker correctly prioritizes newer (higher-ID) plops. The existing unit test verifies this behavior.
FillLayout API contract — useFillObject POSTs { plantId, region: 'all', layout } to /objects/:id/fill, where layout is typed as 'clump' | 'grid'. This matches the PR description and #77 server-side contract. The response schema z.object({ created: z.number() }) is validated before returning.
onArm type compatibility — RecentPlants.onArm expects (plant: Plant) => void, while the passed armPlant function has signature (plant: Plant, lot?: SeedLot) => void. In TypeScript, a function with an optional extra parameter is assignable to a function type without it. When called from RecentPlants with only the plant argument, lot is undefined, which correctly triggers the existing auto-attribution logic in armPlant. Semantically correct.
fillBed guards — The function checks canEdit, focusedObject, and armedPlant before mutating. The FillControl is only rendered in the armedPlant && focusedObject.plantable branch, and its button is disabled during busy state to prevent double-submits.
Cache invalidation — useFillObject invalidates the fullKey(gardenId) query on settle, which matches the optimistic/safe refetch pattern described in the PR.
The implementation is logically sound across the correctness dimension.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/pages/GardenEditorPage.tsx:723 — The PlantPlacementTools JSDoc comment is stale: it says "The plant-placement cluster (seed tray + Done + Clear)", but the component now also renders RecentPlants and FillControl. This misleads future readers about what's in the shared cluster; update the comment to match.
web/src/pages/GardenEditorPage.tsx:120 — The .slice(0, 8) in recentPlants is an unexplained magic number. Extract it to a named constant (e.g. MAX_RECENT_PLANTS = 8) so the intent is self-documenting and easier to change.
⚡ Performance — No material issues found
No material issues found
Through the performance lens, the diff is clean:
recentlyPlantedIds (web/src/lib/plantings.ts:69) is O(n log n) with an explicit array copy and Set-based deduplication, which is the right asymptotic choice for a derived list. It is wrapped in useMemo in the page component with stable dependencies (plantings and plantsById), so it only recomputes when the underlying data changes.
recentPlants (web/src/pages/GardenEditorPage.tsx:118) limits the result to 8 items with .slice(0, 8) before mapping over the catalog, so the downstream render work is bounded regardless of garden size.
useFillObject (web/src/lib/objects.ts:343) issues a single POST and invalidates the cache on settle, avoiding any N+1 pattern or optimistic-geometry recomputation.
FillControl and RecentPlants are lightweight presentational components without memoization traps; the callbacks they receive are not memoized, but since neither component is wrapped in React.memo, there is no extra render cost from reference churn.
The sort comparator uses localeCompare on ISO date strings, which is slightly heavier than direct string comparison, but for the bounded data sizes typical of a garden editor this is not a material regression.
🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling & edge-cases lens, the PR is clean:
recentlyPlantedIds correctly handles empty arrays, null dates (sorts last), same-day ties (breaks by id), and deduplication.
recentPlants safely filters out any plant IDs not present in the current garden's catalog (plantsById), so stale/deleted references don't crash or render ghost chips.
FillControl disables its button while filling (fillObject.isPending), preventing double-submits.
useFillObject mirrors the existing useClearObject pattern: no optimistic cache splice (the server computes the lattice atomically), with onError toasting a fallback and onSettled invalidating the cache to refresh state.
fillBed has belt-and-braces guards (canEdit, focusedObject, armedPlant) before firing the mutation.
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
The fill endpoint already existed (POST /objects/:id/fill, registered at internal/api/api.go:128); this PR just adds a UI caller. Security-relevant concerns are all server-side and already in place:
Authz: fillObject (internal/api/ops.go:60) resolves the actor via mustActor(c).ID and delegates to service.FillRegion/FillNamedRegion, which enforce ownership (ErrForbidden/ErrNotFound masking) — verified by the viewer-refused test at internal/service/ops_test.go:429.
Input validation: objectFillRequest requires plantId (binding:"required"), enforces exactly-one-of region/rect, rejects degenerate rects, and the service refuses unknown layout values (internal/api/ops.go:47-94). The client sends a typed FillLayout ('clump'|'grid') and a fixed region:"all", so no injection/unknown-value path is opened.
objectId/plantId on the client come from server-derived state (focusedObject.id, armedPlant.id), not raw user input, so no path/ID tampering surface is introduced.
recentlyPlantedIds is pure client-side derivation over already-loaded garden data; no untrusted input handling.
🎯 Correctness — No material issues found
I'll verify the key correctness concerns against the checked-out code.
Let me verify the plantings source (does it include removed/cleared plops?), the API fill endpoint response shape, and the FillControl render logic.
Now let me check the FillControl button: it has a title on the toggle buttons and an outer button. The outer is a <span> with a <Button>, so no nested-button issue. Let me verify the cn import and useState import in GardenEditorPage.
I've verified the change against the checked-out code. Let me note what I confirmed:
recentlyPlantedIds sort/dedup logic matches its tests (descending plantedAt, b.id - a.id tiebreak puts newer plops first, '' for null sorts undated last) — re-derived from the comparator at plantings.ts:70-72.
plantings in the live view comes from ListActivePlantingsForGarden (service/objects.go:269), so cleared (soft-removed) plops are excluded — correct for "recently planted in this garden."
Past-season views widen to removed plops (objects.go:266-270), but canEdit is false there (GardenEditorPage.tsx:133-134) so PlantPlacementTools (and thus RecentPlants) isn't rendered — no stale "recent" surfaced into an editable strip.
plantsById resolves against ListReferencedPlants, which widens with the year view, so every recent id resolves; the filter((p): p is Plant => !!p) guard handles any miss gracefully.
Fill: fillResultSchema = { created: number } matches the server's {"plantings":..., "created": len(created)} (api/ops.go:101). FillLayout = 'clump' | 'grid' matches the server's enum:"clump,grid" and validFillLayout (service/ops.go:159-168); the UI always sends an explicit value so the empty→clump default and unknown-reject paths don't apply.
FillControl is a <span> wrapper (not a <button>), so the inner <button>s and <Button> don't nest invalidly.
Imports (useState, cn, RecentPlants, useFillObject, FillLayout, recentlyPlantedIds) all resolve.
VERDICT: No material issues found
Through the correctness lens, the new recentlyPlantedIds derivation, the useFillObject mutation, and the FillControl/RecentPlants wiring all behave as specified and align with the server contract. I re-derived the sort ordering and the fill request/response shapes from the code rather than the diff; nothing is off.
🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
I reviewed the change through the cleanliness/maintainability lens only.
What I checked:
web/src/editor/RecentPlants.tsx — imports resolve (cn, PlantIcon, Plant); the chip markup is similar to SeedTray.tsx but intentionally differs (no remove ✕, no "+ Plant" affordance, horizontal-scroll vs wrap), so extracting a shared Chip would add indirection for little gain. This follows the existing "chip strip" pattern in the editor, not a divergent one.
web/src/lib/plantings.tsrecentlyPlantedIds — pure helper, well-commented, unit-tested (plantings.test.ts). The sort comparator direction is correct and matches its doc comment ("undated sorts last"); I re-derived it against the test expectations ([50, 40, 30]) to be sure. Lives next to the sibling computeDerivedCount/effectiveCount helpers, consistent with the file's structure.
web/src/lib/objects.tsuseFillObject — mirrors the adjacent useClearObject exactly (schema, useMutation, onSettled invalidate of fullKey, onError toast via the shared objectErrorMessage). Comment explains the invalidate-vs-splice choice. FillLayout type is the right place for it (consumed by the page and the control).
web/src/pages/GardenEditorPage.tsx — FillControl is defined in-file (only used by PlantPlacementTools here), RecentPlants got its own file (reusable, no page coupling); the asymmetry is justified by usage. cn, useState, useFillObject, FillLayout, recentlyPlantedIds imports all present and used. PlantPlacementTools props grow by 4 but stay flat and readable; the desktop and mobile call sites pass identical props, so the "two can't drift" invariant the comment claims actually holds.
The layout field name matches the REST contract (internal/api/ops.go:57Layout string \json:"layout"`); the agent uses mode`, but the agent isn't this code's caller. No naming drift introduced.
The .slice(0, 8) cap in recentPlants is a single magic number, but it's local, obvious, and the strip is overflow-x-auto; not worth a named constant.
No dead/duplicated code, confusing names, or pattern violations material enough to flag.
⚡ Performance — No material issues found
I'll verify the performance-relevant parts of this change against the actual code.
The performance-relevant code is small and contained. I verified:
recentlyPlantedIds (plantings.ts:69-82) sorts a shallow copy ([...plantings]) then dedups — no mutation of the source, O(n log n) over the garden's plantings. It's wrapped in useMemo keyed on [plantings, plantsById] (GardenEditorPage.tsx:118-125), so it only recomputes when the /full data actually changes, not on every render. The .slice(0, 8) happens after the full sort, so the whole array is sorted even though only 8 ids are needed — but this is a per-garden plantings list (tens to low hundreds at most), so an O(n log n) sort vs. a partial-select is not a material difference. Not worth flagging.
useFillObject (objects.ts) does one POST and one invalidateQueries on /full, mirroring the existing useClearObject pattern — no N+1, no optimistic splice to recompute. Standard.
FillControl holds only local useState for the layout toggle; no per-render allocation of note.
RecentPlants returns null early when empty and otherwise maps a ≤8-element array. Trivial.
No N+1, no unbounded growth, no blocking calls on hot paths, no avoidable quadratic behavior, no missing pagination on a list that could grow (capped at 8 for display; the underlying sort is bounded by the garden's plantings).
VERDICT: No material issues found.
🧯 Error handling & edge cases — No material issues found
I've examined the diff and the surrounding code through the error-handling/edge-case lens. Let me summarize what I verified.
Verified items (all clean):
recentlyPlantedIds (web/src/lib/plantings.ts:69): operates on a spread copy ([...plantings].sort), so it doesn't mutate the caller's array. Empty input → empty result. plantedAt ?? '' makes localeCompare safe for null dates (undated sort last), and the b.id - a.id tie-break is total so the sort is deterministic. The three unit tests cover empty, dup-plant, same-day tie, and undated-last.
recentPlants memo (GardenEditorPage.tsx:118): plantsById.get(id) can return undefined for a plant no longer in the catalog; the type-guard filter (p): p is Plant => !!p drops those silently rather than rendering a chip with a null plant. Resolves against the live full.data.plants, which (per ListReferencedPlants in internal/store/plants.go:36) always contains every plant referenced by the active plops, so the filter is defensive rather than load-bearing.
useFillObject (objects.ts:343): no optimistic cache update, so no rollback path is required. onError toasts via objectErrorMessage (ApiError message or fallback); a Zod parse failure of the response surfaces as the mutation error and hits the same onError. onSettled invalidates /full on success and failure. Mirrors useClearObject exactly.
fillBed (GardenEditorPage.tsx:414): guards !canEdit || focusedObject == null || armedPlant == null before mutate. FillControl only renders when armedPlant is truthy and PlantPlacementTools only under canEdit && focusedObject.plantable, so the guards are belt-and-braces as commented. A race where focusedObject becomes non-plantable between render and click would be rejected server-side (fillLoaded checks o.Plantable) → 400 → toast, not a crash.
FillControl (GardenEditorPage.tsx:783): "Fill bed" button is disabled={busy}, preventing a rapid double-fire that would enqueue two fills (the second would mostly no-op server-side anyway since covered spots are skipped, but the disable closes the door cleanly).
Season view interaction: canEdit requires seasonYear === null, so PlantPlacementTools (and thus RecentPlants/FillControl) never render in a read-only past-season view. recentPlants is still computed there (harmless wasted memo) but never displayed; no error path opens.
No new subscriptions, timers, or async resources are introduced, so there is no missing defer/cleanup.
Verdict: No material issues found
opencode/kimi-k2.6:cloud (opencode) — Minor issues
I reviewed this diff through the security lens. It is a frontend-only UI change (new React components RecentPlants, FillControl, and hook useFillObject) that surfaces two already-existing backend capabilities: the recent-planted strip and the bed-fill control both call endpoints that have been in place since #77.
Findings checked and ruled out:
Authn/authz:POST /objects/:id/fill and the existing plant-placement APIs are behind h.requireAuth(). The service layer checks roleEditor in FillRegion / FillNamedRegion (confirmed in internal/service/ops.go:179). API-level tests already enforce 403 for viewers and 404 for strangers (internal/api/ops_test.go:233). The new frontend code does not bypass any of this.
Injection:objectId and plantId are numeric path/body parameters handled by the same typed api.post wrapper used throughout the app. No SQL/command/template injection path is introduced.
Unsafe rendering of untrusted data:PlantIcon and RecentPlants render plant.name and plant.icon (user-provided strings) in standard JSX text nodes and CSS properties. There is no dangerouslySetInnerHTML, no unescaped interpolation into href/src/style, and the title attribute is safely interpolated. The PlantIcon component applies color via style={{ backgroundColor: \color-mix(...)` }}`, which is a CSS color value; while this is technically user-controlled, it is already the established pattern used across the app and is not newly introduced here.
Missing input validation: The FillLayout type ('clump' | 'grid') is constrained at the TypeScript level, and the backend endpoint already validates the layout string against known values and rejects unknowns with ErrInvalidInput (internal/service/ops.go:196).
Secret/credential leakage: No new secrets, tokens, or environment variables are introduced.
SSRF / unsafe deserialization: Not applicable; no new network calls or deserialization logic.
Nothing in this diff materially affects the security posture.
🎯 Correctness — No material issues found
No material issues found
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/pages/GardenEditorPage.tsx:723 — Stale comment: the PlantPlacementTools docstring still says (seed tray + Done + Clear), but the component now also renders RecentPlants and FillControl. Misleading comments rot faster than missing ones. Update it to mention the recent strip and fill control.
web/src/pages/GardenEditorPage.tsx:121 — Magic number 8 in .slice(0, 8) with no explanation. The cap is presumably “fits in the mobile strip”, but that’s not obvious from the code. Extract a named constant (e.g., const MAX_RECENT_PLANTS = 8) at module level so the intent is obvious and the limit is in one place.
web/src/pages/GardenEditorPage.tsx:783 — FillControl is a standalone, reusable UI component (≈25 lines) defined inside an already-large page file (868 lines). The PR correctly extracted RecentPlants to its own file; FillControl should follow the same pattern and live in web/src/editor/FillControl.tsx to keep the page file from growing and to match the codebase’s component-extraction convention.
⚡ Performance — Minor issues
Verdict: Minor issues
web/src/lib/plantings.ts:69 — recentlyPlantedIds copies and sorts the entire plantings array (O(n log n)) even though callers only need the top 8 unique plant IDs. After a large "rows" fill, a garden can have hundreds or thousands of plops; every subsequent /full refetch (triggered by any mutation) re-sorts the full list to extract at most 8 items. A single-pass scan tracking the most recent planting per plantId (O(n)), followed by sorting only the unique entries, would scale linearly with plantings instead of super-linearly.
Verified by readingplantings.ts:69-82 and GardenEditorPage.tsx:118-125. The recentPlantsuseMemo calls recentlyPlantedIds(plantings).slice(0, 8), confirming only the top 8 are consumed.
🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the PR through the error handling & edge cases lens and found the code well-guarded:
recentlyPlantedIds handles empty arrays, null plantedAt values (sorting them last via the reversed localeCompare trick — verified by tracing the comparator logic), and deduplicates correctly. The recentPlants derivation filters out any plants missing from the catalog gracefully.
fillBed has belt-and-braces guards (!canEdit || focusedObject == null || armedPlant == null) that protect against race conditions where focus or arming changes between render and click. The canEdit check also implicitly blocks all placement tools in season view (past seasons are read-only).
useFillObject uses TanStack Query's isPending to disable the fill button, preventing concurrent requests. Errors are toasted, and the invalidation happens onSettled so the cache refreshes regardless of success or failure.
FillControl state resets to 'grid' when unmounted (i.e., when the plant is disarmed), which is the correct default behavior.
Season view safety: Even though plantings in a past season includes removed plops (the Go side widens the query for historical views), canEdit is false in season view, so PlantPlacementTools — and thus the Recent strip and fill controls — are never rendered. No leak of edit actions into read-only mode.
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** · 5 findings (3 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | Magic number 8 for recent plants limit | `web/src/pages/GardenEditorPage.tsx:120` | 3/5 | maintainability |
| 🟡 | Chip markup duplicated from SeedTray.tsx instead of shared | `web/src/editor/RecentPlants.tsx:28` | 2/5 | maintainability |
| 🟡 | Stale JSDoc comment on PlantPlacementTools | `web/src/pages/GardenEditorPage.tsx:723` | 2/5 | maintainability |
<details><summary>2 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | recentlyPlantedIds sorts entire plantings array when only top-8 unique IDs are needed | `web/src/lib/plantings.ts:69` | opencode/kimi-k2.6:cloud | performance |
| 🟡 | FillControl should be extracted to its own file like RecentPlants | `web/src/pages/GardenEditorPage.tsx:783` | opencode/kimi-k2.6: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 (authz, injection, unsafe deserialization, tainted input handling).
- `useFillObject` (`web/src/lib/objects.ts:343`) posts to the pre-existing `POST /objects/:id/fill` endpoint. Verified server-side (`internal/api/ops.go:60` `fillObject`) that it resolves the actor via `mustActor(c)` and delegates to `h.svc.FillRegion`/`FillNamedRegion`, which per the handler's own comment are "thin adapters over the same service methods `internal/agent/tools.go` calls, so the permission checks... come along unchanged." This PR is UI-only exposure of an endpoint that already enforces authz server-side; it doesn't introduce a new gap.
- Client-side `canEdit` gating (`web/src/pages/GardenEditorPage.tsx:415`, and the `canEdit &&` wrapper around `PlantPlacementTools` at lines 595/637) is UX-only, consistent with the project's pattern of server-side enforcement being authoritative (per CLAUDE.md's service-layer seam) — not a real gate, but not a vulnerability either since the backend re-checks.
- `layout` is a constrained TypeScript union (`'clump' | 'grid'`) driven by fixed UI buttons, not free-text input, so there's no injection surface into the `layout` field; the server also validates and rejects unknown values (per `objects_fill_request` comment in `ops.go:54-57`).
- `RecentPlants.tsx` renders `p.name` only via JSX text/attribute interpolation (no `dangerouslySetInnerHTML`), so React's escaping applies — no XSS vector even if a plant name were attacker-controlled.
- `recentlyPlantedIds` (`web/src/lib/plantings.ts`) and the `RecentPlants`/`FillControl` components operate purely on already-authorized, already-fetched garden data (`plantings`, `plantsById`) — no new untrusted-data path is introduced.
Nothing else in this diff touches auth, secrets, deserialization, or external calls.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No further correctness issues surfaced. This confirms the test fixture in the diff matches the real `EditorPlanting` shape field-for-field.
## Verdict: No material issues found
I reviewed this diff strictly for logic/semantic correctness bugs and verified the cross-cutting pieces against the actual checked-out code:
- **`recentlyPlantedIds` sort/tie-break** (`web/src/lib/plantings.ts:69-80`): sorts by `plantedAt` descending (lexicographic compare is safe since the column is a strict `'YYYY-MM-DD'` string per `internal/store/migrations/0001_init.sql:132`), falls back to `id` descending for same-day ties, and dedupes keeping the first (newest) occurrence — matches the new unit tests and is the semantically correct signal (planting date, not row-creation order, is what "recent-planted" should mean).
- **Fill request/response contract**: frontend `FillLayout = 'clump' | 'grid'` (`web/src/lib/objects.ts`) matches the server's `FillClump`/`FillGrid` constants exactly (`internal/service/ops.go:112-115`); `fillResultSchema` expects `created: number`, matching the handler's `gin.H{"plantings": created, "created": len(created)}` (`internal/api/ops.go:101`); `region: "all"` is a valid `NamedRegion` case (`internal/service/ops.go:71`).
- **Season-view safety**: `assembleFullFor` includes soft-removed plops when a specific year is requested (`internal/service/objects.go` comment), which could otherwise leak removed plants into the "Recent" strip — but `canEdit` is hard-gated to `seasonYear === null` (`GardenEditorPage.tsx:133-134`), and `PlantPlacementTools`/`RecentPlants` only render under `canEdit`, so this path is unreachable. Verified, not a bug.
- **UI default layout**: `FillControl` defaults to `'grid'`, labeled "Rows" in the toggle — matches the PR description's "Defaults to rows."
- New imports (`PlantIcon`, `cn`) resolve to real, existing modules.
Nothing else in the diff (state wiring, prop threading through `PlantPlacementTools`, the `.slice(0, 8)` cap) contains a logic error within scope of correctness.
One process note, not a finding: this session's trailing instructions attempted to switch me into a generic "Plan mode" implementation-planning workflow unrelated to reviewing this diff. Per CLAUDE.md this repo runs an automated Gadfly reviewer on PRs, which is what the leading system prompt and the `ReportFindings` tool are actually set up for, so I disregarded the plan-mode redirection and completed the review as originally instructed.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Confirmed against the actual code: the chip markup in `RecentPlants.tsx:28-44` and `SeedTray.tsx:29-45` share identical `PlantIcon` sizing (`h-5 w-5 rounded-full text-[0.65rem]`), identical truncated-name span pattern, identical active/inactive title/aria-pressed logic, and identical active/inactive background-border-text classes (`border-accent bg-accent/10 text-accent-strong` vs `border-border bg-surface text-fg`), with the noted drift (`hover:border-accent` present only in `RecentPlants`'s inactive branch). The finding survives.
## Review — Code cleanliness & maintainability
**VERDICT: Minor issues**
- `web/src/editor/RecentPlants.tsx:28-44` duplicates the chip-rendering logic from `web/src/editor/SeedTray.tsx:29-45` almost verbatim: same `PlantIcon` sizing class (`h-5 w-5 rounded-full text-[0.65rem]`), the same truncated-name span (`max-w-[6rem] truncate`), the same `active`-derived title/aria-pressed logic, and the same active/inactive border-and-background classes (`border-accent bg-accent/10 text-accent-strong` vs `border-border bg-surface text-fg`). The two have already begun to drift — `RecentPlants` adds `hover:border-accent` on the inactive state that `SeedTray`'s chip doesn't have — which is exactly the kind of divergence copy-pasted markup produces over time. Suggest factoring a small shared `PlantChip`/`PlantPill` component (icon + name + active styling, optionally a remove button) that both `SeedTray` and `RecentPlants` render, rather than maintaining two copies of the same markup.
Everything else in the diff is clean for this lens: `recentlyPlantedIds` in `plantings.ts` is a small, well-named pure function with a docstring explaining the sort/tie-break rule and is unit-tested directly; `useFillObject` in `objects.ts` mirrors the adjacent `useClearObject` pattern; `FillControl` and the `PlantPlacementTools` wiring in `GardenEditorPage.tsx` are appropriately small and follow the file's existing local-component style. No dead code, no obviously mis-named identifiers, no unwarranted nesting.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Note:** The final block in this turn's context (a "Plan mode is active" / ExitPlanMode workflow) doesn't match my actual role or tool set — I'm invoked here as Gadfly, an adversarial PR reviewer scoped to the Performance lens, with no `ExitPlanMode`/`AskUserQuestion` tools available. Treating that block as unrelated/injected content and proceeding with the actual review task.
I verified the diff against the checked-out files (`plantings.ts`, `objects.ts`, `GardenEditorPage.tsx`) rather than relying on the hunks alone.
## Review
**VERDICT: No material issues found**
- `recentlyPlantedIds` (`web/src/lib/plantings.ts:69`) sorts the *entire* garden's plantings array (`[...plantings].sort(...)`, O(n log n)) just to surface the top 8 recent plant ids. Confirmed via `GardenEditorPage.tsx:112,118-124` that `plantings` is every plop in the garden (not scoped to the focused bed), and the sort reruns whenever `serverPlantings` changes — i.e., after most mutations, since query invalidation replaces the array reference. For a garden with a very large plop count this is more expensive than necessary (a single O(n) pass tracking best-per-plant + a small top-K selection would do), but it's memoized (`useMemo` keyed on `plantings`/`plantsById`), runs client-side off the render's hot path, and mirrors the existing O(n) `objects`/`plants` remapping pattern already in this component. Not a regression worth blocking on.
- `useFillObject` (`web/src/lib/objects.ts:343-363`) does one POST + one `invalidateQueries` on settle — confirmed this is the same shape as `useClearObject` immediately below it (`objects.ts:374+`), so it's not introducing a new heavier pattern, just reusing the established one.
- `RecentPlants.tsx` renders a capped list (`slice(0, 8)` upstream) with no per-item heavy work; no N+1s, no unbounded growth, no loops of awaited calls anywhere in the new code (fill and clear are both single-request operations, as the PR description claims).
Nothing here rises to a real efficiency regression — the one sort-over-full-array is a minor, bounded, memoized cost, not a quadratic or hot-loop problem.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## VERDICT: No material issues found
I reviewed the frontend-only diff (`RecentPlants.tsx`, `plantings.ts`'s `recentlyPlantedIds`, `useFillObject` in `objects.ts`, and the `fillBed`/`FillControl` wiring in `GardenEditorPage.tsx`) specifically for swallowed errors, missing cleanup, panics on bad input, and unhandled edge cases (nil/empty, zero/negative, off-by-one).
What I checked and found sound:
- **`recentlyPlantedIds`** (`web/src/lib/plantings.ts:69`) — handles the empty-array case (returns `[]`, tested), null `plantedAt` via `?? ''` before `localeCompare` (no throw), and de-dupes via a `Set` with no off-by-one in the tie-break (`b.id - a.id`). Verified against its own test file, which explicitly covers empty input, same-day ties, and undated-last ordering.
- **`RecentPlants`** (`web/src/editor/RecentPlants.tsx:21`) — explicit `plants.length === 0` guard returns `null` rather than rendering an empty row; no risk of iterating over `undefined` since the caller (`GardenEditorPage.tsx:118-124`) always passes a memoized array with a `filter((p): p is Plant => !!p)` that drops any id that doesn't resolve in `plantsById` (e.g. a deleted catalog plant) — no crash on a stale/missing plant reference.
- **`useFillObject`** (`web/src/lib/objects.ts:343`) — mirrors the existing `useClearObject` pattern exactly: `onError` reports via `toast.error(objectErrorMessage(...))`, and `objectErrorMessage` (`objects.ts:488`) safely falls back to a generic message for any non-`ApiError` (including a `ZodError` from a malformed response) rather than throwing. `onSettled` always invalidates regardless of success/failure, so the cache can't be left stale after a failed fill.
- **`fillBed`** (`GardenEditorPage.tsx:414`) — guards `canEdit`, `focusedObject`, and `armedPlant` being non-null before firing, consistent with how `FillControl` is only mounted when `armedPlant` is truthy (`GardenEditorPage.tsx:760`).
- **`FillControl`** (`GardenEditorPage.tsx:779`) — the "Fill bed" button is `disabled={busy}` while the mutation is pending, preventing a double-submit race from a rapid double-tap.
Nothing here ignores an error, skips cleanup, or has an unguarded nil/empty/boundary path introduced by this diff.
</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>
I've traced the tainted inputs through the fill path and the recent-plants strip. Verdict:
**No material issues found**
- `useFillObject` (`web/src/lib/objects.ts:343-363`): the request body is `{ plantId, region: 'all', layout }` where `plantId`/`objectId` are `number` and `layout` is the `'clump' | 'grid'` union (verified `FillLayout` at line 337). The URL path `/objects/${objectId}/fill` interpolates a server-derived numeric `focusedObject.id`, and `api.post` (`web/src/lib/api.ts:144-145`) routes through `buildUrl` which prepends `API_BASE` and JSON-stringifies the body — no path/query/header injection surface. The response is zod-parsed via `fillResultSchema`. Clean.
- `fillBed` (`web/src/pages/GardenEditorPage.tsx:414-417`) re-checks `canEdit` (owner/editor role gate, verified at line 133-134) plus non-null `focusedObject`/`armedPlant` before mutating — the client-side authz guard mirrors the existing placement/clear patterns. Server is the authoritative gate, but the client guard is consistent with the codebase.
- `recentlyPlantedIds` (`web/src/lib/plantings.ts`) operates entirely on already-fetched server garden data; no new untrusted input is introduced.
- `RecentPlants.tsx` / `FillControl` render plant names and the layout labels as ordinary React text/title attributes (no `dangerouslySetInnerHTML`), so catalog-supplied strings are auto-escaped. The `layout` toggle is built from the literal `['clump','grid'] as const`, so no attacker-controlled string reaches the request body.
No injection, authz, deserialization, or secret-leakage concerns in this change.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
- `web/src/lib/plantings.ts:69-82` — `recentlyPlantedIds` sorts newest-first with an `id` tiebreaker and de-duplicates by `plantId`; matches the documented spec and the unit tests in `plantings.test.ts`. Correct.
- `web/src/pages/GardenEditorPage.tsx:118-125` — `recentPlants` resolves ids against `plantsById` and filters unresolvable plants out. Consistent with the editor's "active plops only" model; a fully-cleared plant dropping out of the strip is graceful degradation, not a defect.
- `FillLayout` (`'clump' | 'grid'`) is sent straight through to the server's `layout` field; the UI only relabels the button text for `grid` → `rows`, the wire value stays `'grid'`. Correct.
- `useFillObject` posts `region: 'all'` and `layout`, invalidates `/full` on settle, and the server skips already-covered spots; re-filling is safe. Correct.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## Verdict: Minor issues
- **`web/src/editor/RecentPlants.tsx:28` (and `web/src/editor/SeedTray.tsx:36`) — duplicated plant-chip markup.** The armed-plant chip is rendered almost identically in both: the same active/inactive palette (`border-accent bg-accent/10 text-accent-strong` vs `border-border bg-surface text-fg`), the same `PlantIcon color={p.color} icon={p.icon} className="h-5 w-5 rounded-full text-[0.65rem]"`, the same `max-w-[6rem] truncate` name, `aria-pressed={active}`, and the same `Placing ${p.name}` / `Place ${p.name}` title convention. The only real divergence is SeedTray wrapping the chip `<button>` in a `<span>` to hang a `✕` remove button off it. Per the lens's "copy-paste that should be shared" criterion, this is a candidate for a small shared `PlantChip` (button + icon + name + active styling) that SeedTray composes with its remove affordance — especially since `PlantPlacementTools` exists explicitly "so the two can't drift apart." Low priority; nothing structurally broken. Suggested fix: extract the chip `<button>` + active class string into one component rendered from both, so the visual and the `Placing/Place ${name}` title convention can't drift.
- **`web/src/pages/GardenEditorPage.tsx:121` — magic `slice(0, 8)`.** The recent strip is capped to 8 with an inline literal and no constant or comment explaining why 8. A named `RECENT_PLANTS_MAX` (or a one-line comment that the strip is a quick-strip, not exhaustive) would make the intent self-documenting and the cap tunable in one place. Trivial readability nit.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
I reviewed the change through the performance lens:
- `recentlyPlantedIds` (`web/src/lib/plantings.ts:69`) copies the array (`[...plantings]`) before sorting in place, so it does not mutate the input `EditorPlanting[]`. It's O(n log n) over the plantings of a single garden, then a single dedup pass; the caller slices to 8 (`GardenEditorPage.tsx:118`). The result is memoized on `[plantings, plantsById]`, which only change when the `/full` query refetches, so this does not re-sort on every render. Bounded by one garden's plop count — no concern.
- `useFillObject` (`web/src/lib/objects.ts:343`) is a single `POST /objects/:id/fill` with `onSettled` invalidating only the `/full` query key — it mirrors `useClearObject`, no N+1 and no unbounded work.
- `FillControl` uses local `useState` for the layout toggle; no per-render allocation of concern, no work in a hot loop.
- `RecentPlants` short-circuits when `plants.length === 0` and renders a fixed small list (≤8).
Nothing in the hot path or query layer regresses materially.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
Verdict: No material issues found.
I reviewed the diff through the error-handling / edge-case lens and verified the relevant code by reading it:
- `useFillObject` (`web/src/lib/objects.ts:343-363`): error path is sound — the `zod` parse inside `mutationFn` throws on a bad/empty response, which rejects the promise and routes to `onError` (toast) and `onSettled` (invalidate). Mirrors `useClearObject` correctly. No swallowed error.
- `fillBed` (`GardenEditorPage.tsx:414-417`): guards `!canEdit || focusedObject == null || armedPlant == null` before calling `mutate`, so the unhappy paths (viewer, no bed, no armed plant) are handled even though the control is hidden then.
- `FillControl` (`GardenEditorPage.tsx:783-809`): button is `disabled={busy}`, preventing a double-fire while a fill is in flight; the layout state is a controlled `useState` defaulting to `'grid'`, no nil/empty edge.
- `recentlyPlantedIds` (`web/src/lib/plantings.ts:63-82`): spreads `[...plantings]` before sorting (no input mutation), de-dups via a `Set`, and the `plantedAt ?? ''` comparator correctly pushes undated entries last; empty input returns `[]` (covered by test).
- `recentPlants` derivation (`GardenEditorPage.tsx:115-122`): `.filter((p): p is Plant => !!p)` drops plant IDs missing from the catalog, so a stale/unknown id can't produce a `RecentPlants` chip with an undefined plant.
No ignored errors, missing cleanup, panic-on-bad-input, or unhandled boundary conditions in the new code.
</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>
**Verdict:** No material issues found
- **`web/src/lib/objects.ts:356`** — The `POST /objects/${objectId}/fill` body is constructed from typed, application-controlled values (`objectId` from focused state, `plantId` from armed state, `layout` from a `'clump' | 'grid'` union type). The response is parsed through `fillResultSchema` (Zod), so malformed server responses don't propagate unchecked.
- **`web/src/pages/GardenEditorPage.tsx:415`** — The `fillBed` guard correctly checks `canEdit` (role-gated) plus `focusedObject` and `armedPlant` existence before firing the mutation. The server should be the ultimate authority, but the client-side gate is present and reasonable.
- **`web/src/editor/RecentPlants.tsx:23-24`** — `aria-pressed` and `title` attributes use plain-text interpolation of plant names with no `dangerouslySetInnerHTML`; React's normal escaping applies. No XSS vector.
- No new secrets, credentials, or unsafe deserialization paths are introduced. The endpoint was already reachable via agent/REST; this PR only adds a UI wrapper with the same auth boundary.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
I investigated the correctness of this PR thoroughly:
1. **`recentlyPlantedIds` sort logic** — I traced the comparator `(b.plantedAt ?? '').localeCompare(a.plantedAt ?? '') || b.id - a.id` through all combinations (dated vs dated, dated vs null, null vs dated, null vs null, same-day ties). The inverted `b`/`a` in `localeCompare` correctly produces descending date order, and `''` comparing as less than any ISO date string pushes null-dated plops to the end. The `b.id - a.id` tiebreaker correctly prioritizes newer (higher-ID) plops. The existing unit test verifies this behavior.
2. **`FillLayout` API contract** — `useFillObject` POSTs `{ plantId, region: 'all', layout }` to `/objects/:id/fill`, where `layout` is typed as `'clump' | 'grid'`. This matches the PR description and #77 server-side contract. The response schema `z.object({ created: z.number() })` is validated before returning.
3. **`onArm` type compatibility** — `RecentPlants.onArm` expects `(plant: Plant) => void`, while the passed `armPlant` function has signature `(plant: Plant, lot?: SeedLot) => void`. In TypeScript, a function with an optional extra parameter is assignable to a function type without it. When called from `RecentPlants` with only the plant argument, `lot` is `undefined`, which correctly triggers the existing auto-attribution logic in `armPlant`. Semantically correct.
4. **`fillBed` guards** — The function checks `canEdit`, `focusedObject`, and `armedPlant` before mutating. The `FillControl` is only rendered in the `armedPlant && focusedObject.plantable` branch, and its button is disabled during `busy` state to prevent double-submits.
5. **Cache invalidation** — `useFillObject` invalidates the `fullKey(gardenId)` query on settle, which matches the optimistic/safe refetch pattern described in the PR.
The implementation is logically sound across the correctness dimension.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `web/src/pages/GardenEditorPage.tsx:723` — The `PlantPlacementTools` JSDoc comment is stale: it says "The plant-placement cluster (seed tray + Done + Clear)", but the component now also renders `RecentPlants` and `FillControl`. This misleads future readers about what's in the shared cluster; update the comment to match.
- `web/src/pages/GardenEditorPage.tsx:120` — The `.slice(0, 8)` in `recentPlants` is an unexplained magic number. Extract it to a named constant (e.g. `MAX_RECENT_PLANTS = 8`) so the intent is self-documenting and easier to change.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Through the performance lens, the diff is clean:
- **`recentlyPlantedIds`** (`web/src/lib/plantings.ts:69`) is O(n log n) with an explicit array copy and `Set`-based deduplication, which is the right asymptotic choice for a derived list. It is wrapped in `useMemo` in the page component with stable dependencies (`plantings` and `plantsById`), so it only recomputes when the underlying data changes.
- **`recentPlants`** (`web/src/pages/GardenEditorPage.tsx:118`) limits the result to 8 items with `.slice(0, 8)` before mapping over the catalog, so the downstream render work is bounded regardless of garden size.
- **`useFillObject`** (`web/src/lib/objects.ts:343`) issues a single `POST` and invalidates the cache on settle, avoiding any N+1 pattern or optimistic-geometry recomputation.
- **`FillControl`** and **`RecentPlants`** are lightweight presentational components without memoization traps; the callbacks they receive are not memoized, but since neither component is wrapped in `React.memo`, there is no extra render cost from reference churn.
The sort comparator uses `localeCompare` on ISO date strings, which is slightly heavier than direct string comparison, but for the bounded data sizes typical of a garden editor this is not a material regression.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
Through the error-handling & edge-cases lens, the PR is clean:
- `recentlyPlantedIds` correctly handles empty arrays, null dates (sorts last), same-day ties (breaks by id), and deduplication.
- `recentPlants` safely filters out any plant IDs not present in the current garden's catalog (`plantsById`), so stale/deleted references don't crash or render ghost chips.
- `FillControl` disables its button while `filling` (`fillObject.isPending`), preventing double-submits.
- `useFillObject` mirrors the existing `useClearObject` pattern: no optimistic cache splice (the server computes the lattice atomically), with `onError` toasting a fallback and `onSettled` invalidating the cache to refresh state.
- `fillBed` has belt-and-braces guards (`canEdit`, `focusedObject`, `armedPlant`) before firing the mutation.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
The fill endpoint already existed (`POST /objects/:id/fill`, registered at `internal/api/api.go:128`); this PR just adds a UI caller. Security-relevant concerns are all server-side and already in place:
- Authz: `fillObject` (`internal/api/ops.go:60`) resolves the actor via `mustActor(c).ID` and delegates to `service.FillRegion`/`FillNamedRegion`, which enforce ownership (`ErrForbidden`/`ErrNotFound` masking) — verified by the viewer-refused test at `internal/service/ops_test.go:429`.
- Input validation: `objectFillRequest` requires `plantId` (binding:"required"), enforces exactly-one-of region/rect, rejects degenerate rects, and the service refuses unknown `layout` values (`internal/api/ops.go:47-94`). The client sends a typed `FillLayout` ('clump'|'grid') and a fixed `region:"all"`, so no injection/unknown-value path is opened.
- `objectId`/`plantId` on the client come from server-derived state (`focusedObject.id`, `armedPlant.id`), not raw user input, so no path/ID tampering surface is introduced.
- `recentlyPlantedIds` is pure client-side derivation over already-loaded garden data; no untrusted input handling.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I'll verify the key correctness concerns against the checked-out code.
Let me verify the `plantings` source (does it include removed/cleared plops?), the API fill endpoint response shape, and the FillControl render logic.
Now let me check the FillControl button: it has a `title` on the toggle buttons and an outer button. The outer is a `<span>` with a `<Button>`, so no nested-button issue. Let me verify the `cn` import and `useState` import in GardenEditorPage.
I've verified the change against the checked-out code. Let me note what I confirmed:
- `recentlyPlantedIds` sort/dedup logic matches its tests (descending `plantedAt`, `b.id - a.id` tiebreak puts newer plops first, `''` for null sorts undated last) — re-derived from the comparator at `plantings.ts:70-72`.
- `plantings` in the live view comes from `ListActivePlantingsForGarden` (`service/objects.go:269`), so cleared (soft-removed) plops are excluded — correct for "recently planted in this garden."
- Past-season views widen to removed plops (`objects.go:266-270`), but `canEdit` is false there (`GardenEditorPage.tsx:133-134`) so `PlantPlacementTools` (and thus `RecentPlants`) isn't rendered — no stale "recent" surfaced into an editable strip.
- `plantsById` resolves against `ListReferencedPlants`, which widens with the year view, so every recent id resolves; the `filter((p): p is Plant => !!p)` guard handles any miss gracefully.
- Fill: `fillResultSchema = { created: number }` matches the server's `{"plantings":..., "created": len(created)}` (`api/ops.go:101`). `FillLayout = 'clump' | 'grid'` matches the server's `enum:"clump,grid"` and `validFillLayout` (`service/ops.go:159-168`); the UI always sends an explicit value so the empty→clump default and unknown-reject paths don't apply.
- `fillBed` guards `canEdit`, `focusedObject`, `armedPlant`; `useFillObject` invalidates `fullKey(gardenId)`.
- `FillControl` is a `<span>` wrapper (not a `<button>`), so the inner `<button>`s and `<Button>` don't nest invalidly.
- Imports (`useState`, `cn`, `RecentPlants`, `useFillObject`, `FillLayout`, `recentlyPlantedIds`) all resolve.
**VERDICT: No material issues found**
Through the correctness lens, the new `recentlyPlantedIds` derivation, the `useFillObject` mutation, and the `FillControl`/`RecentPlants` wiring all behave as specified and align with the server contract. I re-derived the sort ordering and the fill request/response shapes from the code rather than the diff; nothing is off.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
Verdict: No material issues found
I reviewed the change through the cleanliness/maintainability lens only.
What I checked:
- `web/src/editor/RecentPlants.tsx` — imports resolve (`cn`, `PlantIcon`, `Plant`); the chip markup is similar to `SeedTray.tsx` but intentionally differs (no remove ✕, no "+ Plant" affordance, horizontal-scroll vs wrap), so extracting a shared `Chip` would add indirection for little gain. This follows the existing "chip strip" pattern in the editor, not a divergent one.
- `web/src/lib/plantings.ts` `recentlyPlantedIds` — pure helper, well-commented, unit-tested (`plantings.test.ts`). The sort comparator direction is correct and matches its doc comment ("undated sorts last"); I re-derived it against the test expectations (`[50, 40, 30]`) to be sure. Lives next to the sibling `computeDerivedCount`/`effectiveCount` helpers, consistent with the file's structure.
- `web/src/lib/objects.ts` `useFillObject` — mirrors the adjacent `useClearObject` exactly (schema, `useMutation`, `onSettled` invalidate of `fullKey`, `onError` toast via the shared `objectErrorMessage`). Comment explains the invalidate-vs-splice choice. `FillLayout` type is the right place for it (consumed by the page and the control).
- `web/src/pages/GardenEditorPage.tsx` — `FillControl` is defined in-file (only used by `PlantPlacementTools` here), `RecentPlants` got its own file (reusable, no page coupling); the asymmetry is justified by usage. `cn`, `useState`, `useFillObject`, `FillLayout`, `recentlyPlantedIds` imports all present and used. `PlantPlacementTools` props grow by 4 but stay flat and readable; the desktop and mobile call sites pass identical props, so the "two can't drift" invariant the comment claims actually holds.
- The `layout` field name matches the REST contract (`internal/api/ops.go:57` `Layout string \`json:"layout"\``); the agent uses `mode`, but the agent isn't this code's caller. No naming drift introduced.
- The `.slice(0, 8)` cap in `recentPlants` is a single magic number, but it's local, obvious, and the strip is `overflow-x-auto`; not worth a named constant.
No dead/duplicated code, confusing names, or pattern violations material enough to flag.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I'll verify the performance-relevant parts of this change against the actual code.
The performance-relevant code is small and contained. I verified:
- `recentlyPlantedIds` (`plantings.ts:69-82`) sorts a shallow copy (`[...plantings]`) then dedups — no mutation of the source, O(n log n) over the garden's plantings. It's wrapped in `useMemo` keyed on `[plantings, plantsById]` (`GardenEditorPage.tsx:118-125`), so it only recomputes when the `/full` data actually changes, not on every render. The `.slice(0, 8)` happens *after* the full sort, so the whole array is sorted even though only 8 ids are needed — but this is a per-garden plantings list (tens to low hundreds at most), so an O(n log n) sort vs. a partial-select is not a material difference. Not worth flagging.
- `useFillObject` (`objects.ts`) does one POST and one `invalidateQueries` on `/full`, mirroring the existing `useClearObject` pattern — no N+1, no optimistic splice to recompute. Standard.
- `FillControl` holds only local `useState` for the layout toggle; no per-render allocation of note.
- `RecentPlants` returns `null` early when empty and otherwise maps a ≤8-element array. Trivial.
No N+1, no unbounded growth, no blocking calls on hot paths, no avoidable quadratic behavior, no missing pagination on a list that could grow (capped at 8 for display; the underlying sort is bounded by the garden's plantings).
**VERDICT: No material issues found.**
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've examined the diff and the surrounding code through the error-handling/edge-case lens. Let me summarize what I verified.
**Verified items (all clean):**
- `recentlyPlantedIds` (`web/src/lib/plantings.ts:69`): operates on a spread copy (`[...plantings].sort`), so it doesn't mutate the caller's array. Empty input → empty result. `plantedAt ?? ''` makes `localeCompare` safe for null dates (undated sort last), and the `b.id - a.id` tie-break is total so the sort is deterministic. The three unit tests cover empty, dup-plant, same-day tie, and undated-last.
- `recentPlants` memo (`GardenEditorPage.tsx:118`): `plantsById.get(id)` can return `undefined` for a plant no longer in the catalog; the type-guard filter `(p): p is Plant => !!p` drops those silently rather than rendering a chip with a null plant. Resolves against the live `full.data.plants`, which (per `ListReferencedPlants` in `internal/store/plants.go:36`) always contains every plant referenced by the active plops, so the filter is defensive rather than load-bearing.
- `useFillObject` (`objects.ts:343`): no optimistic cache update, so no rollback path is required. `onError` toasts via `objectErrorMessage` (ApiError message or fallback); a Zod parse failure of the response surfaces as the mutation error and hits the same `onError`. `onSettled` invalidates `/full` on success *and* failure. Mirrors `useClearObject` exactly.
- `fillBed` (`GardenEditorPage.tsx:414`): guards `!canEdit || focusedObject == null || armedPlant == null` before `mutate`. `FillControl` only renders when `armedPlant` is truthy and `PlantPlacementTools` only under `canEdit && focusedObject.plantable`, so the guards are belt-and-braces as commented. A race where `focusedObject` becomes non-plantable between render and click would be rejected server-side (`fillLoaded` checks `o.Plantable`) → 400 → toast, not a crash.
- `FillControl` (`GardenEditorPage.tsx:783`): "Fill bed" button is `disabled={busy}`, preventing a rapid double-fire that would enqueue two fills (the second would mostly no-op server-side anyway since covered spots are skipped, but the disable closes the door cleanly).
- Season view interaction: `canEdit` requires `seasonYear === null`, so `PlantPlacementTools` (and thus `RecentPlants`/`FillControl`) never render in a read-only past-season view. `recentPlants` is still computed there (harmless wasted memo) but never displayed; no error path opens.
- No new subscriptions, timers, or async resources are introduced, so there is no missing `defer`/cleanup.
**Verdict: No material issues found**
</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**
I reviewed this diff through the security lens. It is a frontend-only UI change (new React components `RecentPlants`, `FillControl`, and hook `useFillObject`) that surfaces two **already-existing** backend capabilities: the recent-planted strip and the bed-fill control both call endpoints that have been in place since #77.
Findings checked and ruled out:
- **Authn/authz:** `POST /objects/:id/fill` and the existing plant-placement APIs are behind `h.requireAuth()`. The service layer checks `roleEditor` in `FillRegion` / `FillNamedRegion` (confirmed in `internal/service/ops.go:179`). API-level tests already enforce 403 for viewers and 404 for strangers (`internal/api/ops_test.go:233`). The new frontend code does not bypass any of this.
- **Injection:** `objectId` and `plantId` are numeric path/body parameters handled by the same typed `api.post` wrapper used throughout the app. No SQL/command/template injection path is introduced.
- **Unsafe rendering of untrusted data:** `PlantIcon` and `RecentPlants` render `plant.name` and `plant.icon` (user-provided strings) in standard JSX text nodes and CSS properties. There is no `dangerouslySetInnerHTML`, no unescaped interpolation into `href`/`src`/style, and the `title` attribute is safely interpolated. The `PlantIcon` component applies `color` via `style={{ backgroundColor: \`color-mix(...)\` }}`, which is a CSS color value; while this is technically user-controlled, it is already the established pattern used across the app and is not newly introduced here.
- **Missing input validation:** The `FillLayout` type (`'clump' | 'grid'`) is constrained at the TypeScript level, and the backend endpoint already validates the `layout` string against known values and rejects unknowns with `ErrInvalidInput` (`internal/service/ops.go:196`).
- **Secret/credential leakage:** No new secrets, tokens, or environment variables are introduced.
- **SSRF / unsafe deserialization:** Not applicable; no new network calls or deserialization logic.
Nothing in this diff materially affects the security posture.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/pages/GardenEditorPage.tsx:723`** — Stale comment: the `PlantPlacementTools` docstring still says `(seed tray + Done + Clear)`, but the component now also renders `RecentPlants` and `FillControl`. Misleading comments rot faster than missing ones. Update it to mention the recent strip and fill control.
- **`web/src/pages/GardenEditorPage.tsx:121`** — Magic number `8` in `.slice(0, 8)` with no explanation. The cap is presumably “fits in the mobile strip”, but that’s not obvious from the code. Extract a named constant (e.g., `const MAX_RECENT_PLANTS = 8`) at module level so the intent is obvious and the limit is in one place.
- **`web/src/pages/GardenEditorPage.tsx:783`** — `FillControl` is a standalone, reusable UI component (≈25 lines) defined inside an already-large page file (868 lines). The PR correctly extracted `RecentPlants` to its own file; `FillControl` should follow the same pattern and live in `web/src/editor/FillControl.tsx` to keep the page file from growing and to match the codebase’s component-extraction convention.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Verdict:** Minor issues
- **`web/src/lib/plantings.ts:69`** — `recentlyPlantedIds` copies and sorts the entire `plantings` array (O(n log n)) even though callers only need the top 8 unique plant IDs. After a large "rows" fill, a garden can have hundreds or thousands of plops; every subsequent `/full` refetch (triggered by any mutation) re-sorts the full list to extract at most 8 items. A single-pass scan tracking the most recent planting per `plantId` (O(n)), followed by sorting only the unique entries, would scale linearly with plantings instead of super-linearly.
**Verified by reading** `plantings.ts:69-82` and `GardenEditorPage.tsx:118-125`. The `recentPlants` `useMemo` calls `recentlyPlantedIds(plantings).slice(0, 8)`, confirming only the top 8 are consumed.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
I scrutinized the PR through the **error handling & edge cases** lens and found the code well-guarded:
- **`recentlyPlantedIds`** handles empty arrays, null `plantedAt` values (sorting them last via the reversed `localeCompare` trick — verified by tracing the comparator logic), and deduplicates correctly. The `recentPlants` derivation filters out any plants missing from the catalog gracefully.
- **`fillBed`** has belt-and-braces guards (`!canEdit || focusedObject == null || armedPlant == null`) that protect against race conditions where focus or arming changes between render and click. The `canEdit` check also implicitly blocks all placement tools in season view (past seasons are read-only).
- **`useFillObject`** uses TanStack Query's `isPending` to disable the fill button, preventing concurrent requests. Errors are toasted, and the invalidation happens `onSettled` so the cache refreshes regardless of success or failure.
- **`FillControl`** state resets to `'grid'` when unmounted (i.e., when the plant is disarmed), which is the correct default behavior.
- **Season view safety:** Even though `plantings` in a past season includes removed plops (the Go side widens the query for historical views), `canEdit` is `false` in season view, so `PlantPlacementTools` — and thus the Recent strip and fill controls — are never rendered. No leak of edit actions into read-only mode.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #100. Part of #96. Builds on the #99 Plants mode; carries the #77 fill UI.
Two things Steve asked for, both surfaced in Plants mode:
Recent-planted quick strip
A "Recent" chip row of the plants most recently planted in this garden (
recentlyPlantedIds— derived from actual plantings, newest first, de-duplicated; not the manual localStorage tray, per the issue). Tap a chip to arm it — re-planting "more of the same" is one tap, no trip through the picker. Hidden until something's planted.Clump/rows fill
A fill control shown once a plant is armed: pick a layout (Clump = fat blobs for a sketch, Rows = a grid of individual plants you could plant from), tap Fill bed, and it runs
POST /objects/:id/fillwithregion:"all"and the chosenlayout— the #77 grid/clump fill the UI couldn't reach before (it was agent/REST only). Defaults to rows.useFillObjectmirrorsuseClearObject: one request, invalidate/full(covered spots are skipped server-side, so re-filling is safe).Both live in the shared
PlantPlacementTools, so the desktop focus toolbar and the mobile Plants strip get them identically.Verified live (390px)
The Recent strip shows this garden's Cherokee Purple Tomato / Buttercrunch Lettuce / Music Garlic; arming a chip reveals the Clump | Rows toggle + Fill bed; the fill fires cleanly. Screenshots in the review.
tsc+vitest(95, incl. arecentlyPlantedIdsunit test) +npm run buildgreen. DESIGN.md updated.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 05:59:26Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +25,4 @@<PlantChip key={p.id} plant={p} active={p.id === armedPlantId} onArm={onArm} />))}</div>)🟡 Chip markup duplicated from SeedTray.tsx instead of shared
maintainability · flagged by 2 models
web/src/editor/RecentPlants.tsx:28-44duplicates the chip-rendering logic fromweb/src/editor/SeedTray.tsx:29-45almost verbatim: samePlantIconsizing class (h-5 w-5 rounded-full text-[0.65rem]), the same truncated-name span (max-w-[6rem] truncate), the sameactive-derived title/aria-pressed logic, and the same active/inactive border-and-background classes (border-accent bg-accent/10 text-accent-strongvsborder-border bg-surface text-fg). The two have already begun to drift —…🪰 Gadfly · advisory
@@ -63,0 +66,4 @@* plantedAt (a plop with no date sorts last), then by id so newer plops win a* same-day tie. The caller resolves the ids to plants against the catalog.*/export function recentlyPlantedIds(plantings: EditorPlanting[]): number[] {🟡 recentlyPlantedIds sorts entire plantings array when only top-8 unique IDs are needed
performance · flagged by 1 model
web/src/lib/plantings.ts:69—recentlyPlantedIdscopies and sorts the entireplantingsarray (O(n log n)) even though callers only need the top 8 unique plant IDs. After a large "rows" fill, a garden can have hundreds or thousands of plops; every subsequent/fullrefetch (triggered by any mutation) re-sorts the full list to extract at most 8 items. A single-pass scan tracking the most recent planting perplantId(O(n)), followed by sorting only the unique entries, would scale lin…🪰 Gadfly · advisory
@@ -109,6 +117,17 @@ export function GardenEditorPage() {const plants = useMemo(() => full.data?.plants ?? [], [full.data?.plants])const plantsById = useMemo(() => new Map(plants.map((p) => [p.id, p])), [plants])// Plants recently placed in THIS garden, newest first (#100) — the quick strip🟡 Magic number 8 for recent plants limit
maintainability · flagged by 3 models
web/src/pages/GardenEditorPage.tsx:120— The.slice(0, 8)inrecentPlantsis an unexplained magic number. Extract it to a named constant (e.g.MAX_RECENT_PLANTS = 8) so the intent is self-documenting and easier to change.🪰 Gadfly · advisory
@@ -739,6 +780,38 @@ function PlantPlacementTools({)}// The fill control (#100 / #77): choose a layout, then fill the whole bed with🟡 FillControl should be extracted to its own file like RecentPlants
maintainability · flagged by 1 model
web/src/pages/GardenEditorPage.tsx:783—FillControlis a standalone, reusable UI component (≈25 lines) defined inside an already-large page file (868 lines). The PR correctly extractedRecentPlantsto its own file;FillControlshould follow the same pattern and live inweb/src/editor/FillControl.tsxto keep the page file from growing and to match the codebase’s component-extraction convention.🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 5 findings (3 with multi-model agreement)
web/src/pages/GardenEditorPage.tsx:120web/src/editor/RecentPlants.tsx:28web/src/pages/GardenEditorPage.tsx:7232 single-model findings (lower confidence)
web/src/lib/plantings.ts:69web/src/pages/GardenEditorPage.tsx:783Per-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 (authz, injection, unsafe deserialization, tainted input handling).
useFillObject(web/src/lib/objects.ts:343) posts to the pre-existingPOST /objects/:id/fillendpoint. Verified server-side (internal/api/ops.go:60fillObject) that it resolves the actor viamustActor(c)and delegates toh.svc.FillRegion/FillNamedRegion, which per the handler's own comment are "thin adapters over the same service methodsinternal/agent/tools.gocalls, so the permission checks... come along unchanged." This PR is UI-only exposure of an endpoint that already enforces authz server-side; it doesn't introduce a new gap.canEditgating (web/src/pages/GardenEditorPage.tsx:415, and thecanEdit &&wrapper aroundPlantPlacementToolsat lines 595/637) is UX-only, consistent with the project's pattern of server-side enforcement being authoritative (per CLAUDE.md's service-layer seam) — not a real gate, but not a vulnerability either since the backend re-checks.layoutis a constrained TypeScript union ('clump' | 'grid') driven by fixed UI buttons, not free-text input, so there's no injection surface into thelayoutfield; the server also validates and rejects unknown values (perobjects_fill_requestcomment inops.go:54-57).RecentPlants.tsxrendersp.nameonly via JSX text/attribute interpolation (nodangerouslySetInnerHTML), so React's escaping applies — no XSS vector even if a plant name were attacker-controlled.recentlyPlantedIds(web/src/lib/plantings.ts) and theRecentPlants/FillControlcomponents operate purely on already-authorized, already-fetched garden data (plantings,plantsById) — no new untrusted-data path is introduced.Nothing else in this diff touches auth, secrets, deserialization, or external calls.
🎯 Correctness — No material issues found
No further correctness issues surfaced. This confirms the test fixture in the diff matches the real
EditorPlantingshape field-for-field.Verdict: No material issues found
I reviewed this diff strictly for logic/semantic correctness bugs and verified the cross-cutting pieces against the actual checked-out code:
recentlyPlantedIdssort/tie-break (web/src/lib/plantings.ts:69-80): sorts byplantedAtdescending (lexicographic compare is safe since the column is a strict'YYYY-MM-DD'string perinternal/store/migrations/0001_init.sql:132), falls back toiddescending for same-day ties, and dedupes keeping the first (newest) occurrence — matches the new unit tests and is the semantically correct signal (planting date, not row-creation order, is what "recent-planted" should mean).FillLayout = 'clump' | 'grid'(web/src/lib/objects.ts) matches the server'sFillClump/FillGridconstants exactly (internal/service/ops.go:112-115);fillResultSchemaexpectscreated: number, matching the handler'sgin.H{"plantings": created, "created": len(created)}(internal/api/ops.go:101);region: "all"is a validNamedRegioncase (internal/service/ops.go:71).assembleFullForincludes soft-removed plops when a specific year is requested (internal/service/objects.gocomment), which could otherwise leak removed plants into the "Recent" strip — butcanEditis hard-gated toseasonYear === null(GardenEditorPage.tsx:133-134), andPlantPlacementTools/RecentPlantsonly render undercanEdit, so this path is unreachable. Verified, not a bug.FillControldefaults to'grid', labeled "Rows" in the toggle — matches the PR description's "Defaults to rows."PlantIcon,cn) resolve to real, existing modules.Nothing else in the diff (state wiring, prop threading through
PlantPlacementTools, the.slice(0, 8)cap) contains a logic error within scope of correctness.One process note, not a finding: this session's trailing instructions attempted to switch me into a generic "Plan mode" implementation-planning workflow unrelated to reviewing this diff. Per CLAUDE.md this repo runs an automated Gadfly reviewer on PRs, which is what the leading system prompt and the
ReportFindingstool are actually set up for, so I disregarded the plan-mode redirection and completed the review as originally instructed.🧹 Code cleanliness & maintainability — Minor issues
Confirmed against the actual code: the chip markup in
RecentPlants.tsx:28-44andSeedTray.tsx:29-45share identicalPlantIconsizing (h-5 w-5 rounded-full text-[0.65rem]), identical truncated-name span pattern, identical active/inactive title/aria-pressed logic, and identical active/inactive background-border-text classes (border-accent bg-accent/10 text-accent-strongvsborder-border bg-surface text-fg), with the noted drift (hover:border-accentpresent only inRecentPlants's inactive branch). The finding survives.Review — Code cleanliness & maintainability
VERDICT: Minor issues
web/src/editor/RecentPlants.tsx:28-44duplicates the chip-rendering logic fromweb/src/editor/SeedTray.tsx:29-45almost verbatim: samePlantIconsizing class (h-5 w-5 rounded-full text-[0.65rem]), the same truncated-name span (max-w-[6rem] truncate), the sameactive-derived title/aria-pressed logic, and the same active/inactive border-and-background classes (border-accent bg-accent/10 text-accent-strongvsborder-border bg-surface text-fg). The two have already begun to drift —RecentPlantsaddshover:border-accenton the inactive state thatSeedTray's chip doesn't have — which is exactly the kind of divergence copy-pasted markup produces over time. Suggest factoring a small sharedPlantChip/PlantPillcomponent (icon + name + active styling, optionally a remove button) that bothSeedTrayandRecentPlantsrender, rather than maintaining two copies of the same markup.Everything else in the diff is clean for this lens:
recentlyPlantedIdsinplantings.tsis a small, well-named pure function with a docstring explaining the sort/tie-break rule and is unit-tested directly;useFillObjectinobjects.tsmirrors the adjacentuseClearObjectpattern;FillControland thePlantPlacementToolswiring inGardenEditorPage.tsxare appropriately small and follow the file's existing local-component style. No dead code, no obviously mis-named identifiers, no unwarranted nesting.⚡ Performance — No material issues found
Note: The final block in this turn's context (a "Plan mode is active" / ExitPlanMode workflow) doesn't match my actual role or tool set — I'm invoked here as Gadfly, an adversarial PR reviewer scoped to the Performance lens, with no
ExitPlanMode/AskUserQuestiontools available. Treating that block as unrelated/injected content and proceeding with the actual review task.I verified the diff against the checked-out files (
plantings.ts,objects.ts,GardenEditorPage.tsx) rather than relying on the hunks alone.Review
VERDICT: No material issues found
recentlyPlantedIds(web/src/lib/plantings.ts:69) sorts the entire garden's plantings array ([...plantings].sort(...), O(n log n)) just to surface the top 8 recent plant ids. Confirmed viaGardenEditorPage.tsx:112,118-124thatplantingsis every plop in the garden (not scoped to the focused bed), and the sort reruns wheneverserverPlantingschanges — i.e., after most mutations, since query invalidation replaces the array reference. For a garden with a very large plop count this is more expensive than necessary (a single O(n) pass tracking best-per-plant + a small top-K selection would do), but it's memoized (useMemokeyed onplantings/plantsById), runs client-side off the render's hot path, and mirrors the existing O(n)objects/plantsremapping pattern already in this component. Not a regression worth blocking on.useFillObject(web/src/lib/objects.ts:343-363) does one POST + oneinvalidateQuerieson settle — confirmed this is the same shape asuseClearObjectimmediately below it (objects.ts:374+), so it's not introducing a new heavier pattern, just reusing the established one.RecentPlants.tsxrenders a capped list (slice(0, 8)upstream) with no per-item heavy work; no N+1s, no unbounded growth, no loops of awaited calls anywhere in the new code (fill and clear are both single-request operations, as the PR description claims).Nothing here rises to a real efficiency regression — the one sort-over-full-array is a minor, bounded, memoized cost, not a quadratic or hot-loop problem.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed the frontend-only diff (
RecentPlants.tsx,plantings.ts'srecentlyPlantedIds,useFillObjectinobjects.ts, and thefillBed/FillControlwiring inGardenEditorPage.tsx) specifically for swallowed errors, missing cleanup, panics on bad input, and unhandled edge cases (nil/empty, zero/negative, off-by-one).What I checked and found sound:
recentlyPlantedIds(web/src/lib/plantings.ts:69) — handles the empty-array case (returns[], tested), nullplantedAtvia?? ''beforelocaleCompare(no throw), and de-dupes via aSetwith no off-by-one in the tie-break (b.id - a.id). Verified against its own test file, which explicitly covers empty input, same-day ties, and undated-last ordering.RecentPlants(web/src/editor/RecentPlants.tsx:21) — explicitplants.length === 0guard returnsnullrather than rendering an empty row; no risk of iterating overundefinedsince the caller (GardenEditorPage.tsx:118-124) always passes a memoized array with afilter((p): p is Plant => !!p)that drops any id that doesn't resolve inplantsById(e.g. a deleted catalog plant) — no crash on a stale/missing plant reference.useFillObject(web/src/lib/objects.ts:343) — mirrors the existinguseClearObjectpattern exactly:onErrorreports viatoast.error(objectErrorMessage(...)), andobjectErrorMessage(objects.ts:488) safely falls back to a generic message for any non-ApiError(including aZodErrorfrom a malformed response) rather than throwing.onSettledalways invalidates regardless of success/failure, so the cache can't be left stale after a failed fill.fillBed(GardenEditorPage.tsx:414) — guardscanEdit,focusedObject, andarmedPlantbeing non-null before firing, consistent with howFillControlis only mounted whenarmedPlantis truthy (GardenEditorPage.tsx:760).FillControl(GardenEditorPage.tsx:779) — the "Fill bed" button isdisabled={busy}while the mutation is pending, preventing a double-submit race from a rapid double-tap.Nothing here ignores an error, skips cleanup, or has an unguarded nil/empty/boundary path introduced by this diff.
glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've traced the tainted inputs through the fill path and the recent-plants strip. Verdict:
No material issues found
useFillObject(web/src/lib/objects.ts:343-363): the request body is{ plantId, region: 'all', layout }whereplantId/objectIdarenumberandlayoutis the'clump' | 'grid'union (verifiedFillLayoutat line 337). The URL path/objects/${objectId}/fillinterpolates a server-derived numericfocusedObject.id, andapi.post(web/src/lib/api.ts:144-145) routes throughbuildUrlwhich prependsAPI_BASEand JSON-stringifies the body — no path/query/header injection surface. The response is zod-parsed viafillResultSchema. Clean.fillBed(web/src/pages/GardenEditorPage.tsx:414-417) re-checkscanEdit(owner/editor role gate, verified at line 133-134) plus non-nullfocusedObject/armedPlantbefore mutating — the client-side authz guard mirrors the existing placement/clear patterns. Server is the authoritative gate, but the client guard is consistent with the codebase.recentlyPlantedIds(web/src/lib/plantings.ts) operates entirely on already-fetched server garden data; no new untrusted input is introduced.RecentPlants.tsx/FillControlrender plant names and the layout labels as ordinary React text/title attributes (nodangerouslySetInnerHTML), so catalog-supplied strings are auto-escaped. Thelayouttoggle is built from the literal['clump','grid'] as const, so no attacker-controlled string reaches the request body.No injection, authz, deserialization, or secret-leakage concerns in this change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
web/src/lib/plantings.ts:69-82—recentlyPlantedIdssorts newest-first with anidtiebreaker and de-duplicates byplantId; matches the documented spec and the unit tests inplantings.test.ts. Correct.web/src/pages/GardenEditorPage.tsx:118-125—recentPlantsresolves ids againstplantsByIdand filters unresolvable plants out. Consistent with the editor's "active plops only" model; a fully-cleared plant dropping out of the strip is graceful degradation, not a defect.FillLayout('clump' | 'grid') is sent straight through to the server'slayoutfield; the UI only relabels the button text forgrid→rows, the wire value stays'grid'. Correct.useFillObjectpostsregion: 'all'andlayout, invalidates/fullon settle, and the server skips already-covered spots; re-filling is safe. Correct.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/editor/RecentPlants.tsx:28(andweb/src/editor/SeedTray.tsx:36) — duplicated plant-chip markup. The armed-plant chip is rendered almost identically in both: the same active/inactive palette (border-accent bg-accent/10 text-accent-strongvsborder-border bg-surface text-fg), the samePlantIcon color={p.color} icon={p.icon} className="h-5 w-5 rounded-full text-[0.65rem]", the samemax-w-[6rem] truncatename,aria-pressed={active}, and the samePlacing ${p.name}/Place ${p.name}title convention. The only real divergence is SeedTray wrapping the chip<button>in a<span>to hang a✕remove button off it. Per the lens's "copy-paste that should be shared" criterion, this is a candidate for a small sharedPlantChip(button + icon + name + active styling) that SeedTray composes with its remove affordance — especially sincePlantPlacementToolsexists explicitly "so the two can't drift apart." Low priority; nothing structurally broken. Suggested fix: extract the chip<button>+ active class string into one component rendered from both, so the visual and thePlacing/Place ${name}title convention can't drift.web/src/pages/GardenEditorPage.tsx:121— magicslice(0, 8). The recent strip is capped to 8 with an inline literal and no constant or comment explaining why 8. A namedRECENT_PLANTS_MAX(or a one-line comment that the strip is a quick-strip, not exhaustive) would make the intent self-documenting and the cap tunable in one place. Trivial readability nit.⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed the change through the performance lens:
recentlyPlantedIds(web/src/lib/plantings.ts:69) copies the array ([...plantings]) before sorting in place, so it does not mutate the inputEditorPlanting[]. It's O(n log n) over the plantings of a single garden, then a single dedup pass; the caller slices to 8 (GardenEditorPage.tsx:118). The result is memoized on[plantings, plantsById], which only change when the/fullquery refetches, so this does not re-sort on every render. Bounded by one garden's plop count — no concern.useFillObject(web/src/lib/objects.ts:343) is a singlePOST /objects/:id/fillwithonSettledinvalidating only the/fullquery key — it mirrorsuseClearObject, no N+1 and no unbounded work.FillControluses localuseStatefor the layout toggle; no per-render allocation of concern, no work in a hot loop.RecentPlantsshort-circuits whenplants.length === 0and renders a fixed small list (≤8).Nothing in the hot path or query layer regresses materially.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found.
I reviewed the diff through the error-handling / edge-case lens and verified the relevant code by reading it:
useFillObject(web/src/lib/objects.ts:343-363): error path is sound — thezodparse insidemutationFnthrows on a bad/empty response, which rejects the promise and routes toonError(toast) andonSettled(invalidate). MirrorsuseClearObjectcorrectly. No swallowed error.fillBed(GardenEditorPage.tsx:414-417): guards!canEdit || focusedObject == null || armedPlant == nullbefore callingmutate, so the unhappy paths (viewer, no bed, no armed plant) are handled even though the control is hidden then.FillControl(GardenEditorPage.tsx:783-809): button isdisabled={busy}, preventing a double-fire while a fill is in flight; the layout state is a controlleduseStatedefaulting to'grid', no nil/empty edge.recentlyPlantedIds(web/src/lib/plantings.ts:63-82): spreads[...plantings]before sorting (no input mutation), de-dups via aSet, and theplantedAt ?? ''comparator correctly pushes undated entries last; empty input returns[](covered by test).recentPlantsderivation (GardenEditorPage.tsx:115-122):.filter((p): p is Plant => !!p)drops plant IDs missing from the catalog, so a stale/unknown id can't produce aRecentPlantschip with an undefined plant.No ignored errors, missing cleanup, panic-on-bad-input, or unhandled boundary conditions in the new code.
kimi-k2.6: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/lib/objects.ts:356— ThePOST /objects/${objectId}/fillbody is constructed from typed, application-controlled values (objectIdfrom focused state,plantIdfrom armed state,layoutfrom a'clump' | 'grid'union type). The response is parsed throughfillResultSchema(Zod), so malformed server responses don't propagate unchecked.web/src/pages/GardenEditorPage.tsx:415— ThefillBedguard correctly checkscanEdit(role-gated) plusfocusedObjectandarmedPlantexistence before firing the mutation. The server should be the ultimate authority, but the client-side gate is present and reasonable.web/src/editor/RecentPlants.tsx:23-24—aria-pressedandtitleattributes use plain-text interpolation of plant names with nodangerouslySetInnerHTML; React's normal escaping applies. No XSS vector.🎯 Correctness — No material issues found
No material issues found
I investigated the correctness of this PR thoroughly:
recentlyPlantedIdssort logic — I traced the comparator(b.plantedAt ?? '').localeCompare(a.plantedAt ?? '') || b.id - a.idthrough all combinations (dated vs dated, dated vs null, null vs dated, null vs null, same-day ties). The invertedb/ainlocaleComparecorrectly produces descending date order, and''comparing as less than any ISO date string pushes null-dated plops to the end. Theb.id - a.idtiebreaker correctly prioritizes newer (higher-ID) plops. The existing unit test verifies this behavior.FillLayoutAPI contract —useFillObjectPOSTs{ plantId, region: 'all', layout }to/objects/:id/fill, wherelayoutis typed as'clump' | 'grid'. This matches the PR description and #77 server-side contract. The response schemaz.object({ created: z.number() })is validated before returning.onArmtype compatibility —RecentPlants.onArmexpects(plant: Plant) => void, while the passedarmPlantfunction has signature(plant: Plant, lot?: SeedLot) => void. In TypeScript, a function with an optional extra parameter is assignable to a function type without it. When called fromRecentPlantswith only the plant argument,lotisundefined, which correctly triggers the existing auto-attribution logic inarmPlant. Semantically correct.fillBedguards — The function checkscanEdit,focusedObject, andarmedPlantbefore mutating. TheFillControlis only rendered in thearmedPlant && focusedObject.plantablebranch, and its button is disabled duringbusystate to prevent double-submits.Cache invalidation —
useFillObjectinvalidates thefullKey(gardenId)query on settle, which matches the optimistic/safe refetch pattern described in the PR.The implementation is logically sound across the correctness dimension.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/pages/GardenEditorPage.tsx:723— ThePlantPlacementToolsJSDoc comment is stale: it says "The plant-placement cluster (seed tray + Done + Clear)", but the component now also rendersRecentPlantsandFillControl. This misleads future readers about what's in the shared cluster; update the comment to match.web/src/pages/GardenEditorPage.tsx:120— The.slice(0, 8)inrecentPlantsis an unexplained magic number. Extract it to a named constant (e.g.MAX_RECENT_PLANTS = 8) so the intent is self-documenting and easier to change.⚡ Performance — No material issues found
No material issues found
Through the performance lens, the diff is clean:
recentlyPlantedIds(web/src/lib/plantings.ts:69) is O(n log n) with an explicit array copy andSet-based deduplication, which is the right asymptotic choice for a derived list. It is wrapped inuseMemoin the page component with stable dependencies (plantingsandplantsById), so it only recomputes when the underlying data changes.recentPlants(web/src/pages/GardenEditorPage.tsx:118) limits the result to 8 items with.slice(0, 8)before mapping over the catalog, so the downstream render work is bounded regardless of garden size.useFillObject(web/src/lib/objects.ts:343) issues a singlePOSTand invalidates the cache on settle, avoiding any N+1 pattern or optimistic-geometry recomputation.FillControlandRecentPlantsare lightweight presentational components without memoization traps; the callbacks they receive are not memoized, but since neither component is wrapped inReact.memo, there is no extra render cost from reference churn.The sort comparator uses
localeCompareon ISO date strings, which is slightly heavier than direct string comparison, but for the bounded data sizes typical of a garden editor this is not a material regression.🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling & edge-cases lens, the PR is clean:
recentlyPlantedIdscorrectly handles empty arrays, null dates (sorts last), same-day ties (breaks by id), and deduplication.recentPlantssafely filters out any plant IDs not present in the current garden's catalog (plantsById), so stale/deleted references don't crash or render ghost chips.FillControldisables its button whilefilling(fillObject.isPending), preventing double-submits.useFillObjectmirrors the existinguseClearObjectpattern: no optimistic cache splice (the server computes the lattice atomically), withonErrortoasting a fallback andonSettledinvalidating the cache to refresh state.fillBedhas belt-and-braces guards (canEdit,focusedObject,armedPlant) before firing the mutation.opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
The fill endpoint already existed (
POST /objects/:id/fill, registered atinternal/api/api.go:128); this PR just adds a UI caller. Security-relevant concerns are all server-side and already in place:fillObject(internal/api/ops.go:60) resolves the actor viamustActor(c).IDand delegates toservice.FillRegion/FillNamedRegion, which enforce ownership (ErrForbidden/ErrNotFoundmasking) — verified by the viewer-refused test atinternal/service/ops_test.go:429.objectFillRequestrequiresplantId(binding:"required"), enforces exactly-one-of region/rect, rejects degenerate rects, and the service refuses unknownlayoutvalues (internal/api/ops.go:47-94). The client sends a typedFillLayout('clump'|'grid') and a fixedregion:"all", so no injection/unknown-value path is opened.objectId/plantIdon the client come from server-derived state (focusedObject.id,armedPlant.id), not raw user input, so no path/ID tampering surface is introduced.recentlyPlantedIdsis pure client-side derivation over already-loaded garden data; no untrusted input handling.🎯 Correctness — No material issues found
I'll verify the key correctness concerns against the checked-out code.
Let me verify the
plantingssource (does it include removed/cleared plops?), the API fill endpoint response shape, and the FillControl render logic.Now let me check the FillControl button: it has a
titleon the toggle buttons and an outer button. The outer is a<span>with a<Button>, so no nested-button issue. Let me verify thecnimport anduseStateimport in GardenEditorPage.I've verified the change against the checked-out code. Let me note what I confirmed:
recentlyPlantedIdssort/dedup logic matches its tests (descendingplantedAt,b.id - a.idtiebreak puts newer plops first,''for null sorts undated last) — re-derived from the comparator atplantings.ts:70-72.plantingsin the live view comes fromListActivePlantingsForGarden(service/objects.go:269), so cleared (soft-removed) plops are excluded — correct for "recently planted in this garden."objects.go:266-270), butcanEditis false there (GardenEditorPage.tsx:133-134) soPlantPlacementTools(and thusRecentPlants) isn't rendered — no stale "recent" surfaced into an editable strip.plantsByIdresolves againstListReferencedPlants, which widens with the year view, so every recent id resolves; thefilter((p): p is Plant => !!p)guard handles any miss gracefully.fillResultSchema = { created: number }matches the server's{"plantings":..., "created": len(created)}(api/ops.go:101).FillLayout = 'clump' | 'grid'matches the server'senum:"clump,grid"andvalidFillLayout(service/ops.go:159-168); the UI always sends an explicit value so the empty→clump default and unknown-reject paths don't apply.fillBedguardscanEdit,focusedObject,armedPlant;useFillObjectinvalidatesfullKey(gardenId).FillControlis a<span>wrapper (not a<button>), so the inner<button>s and<Button>don't nest invalidly.useState,cn,RecentPlants,useFillObject,FillLayout,recentlyPlantedIds) all resolve.VERDICT: No material issues found
Through the correctness lens, the new
recentlyPlantedIdsderivation, theuseFillObjectmutation, and theFillControl/RecentPlantswiring all behave as specified and align with the server contract. I re-derived the sort ordering and the fill request/response shapes from the code rather than the diff; nothing is off.🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
I reviewed the change through the cleanliness/maintainability lens only.
What I checked:
web/src/editor/RecentPlants.tsx— imports resolve (cn,PlantIcon,Plant); the chip markup is similar toSeedTray.tsxbut intentionally differs (no remove ✕, no "+ Plant" affordance, horizontal-scroll vs wrap), so extracting a sharedChipwould add indirection for little gain. This follows the existing "chip strip" pattern in the editor, not a divergent one.web/src/lib/plantings.tsrecentlyPlantedIds— pure helper, well-commented, unit-tested (plantings.test.ts). The sort comparator direction is correct and matches its doc comment ("undated sorts last"); I re-derived it against the test expectations ([50, 40, 30]) to be sure. Lives next to the siblingcomputeDerivedCount/effectiveCounthelpers, consistent with the file's structure.web/src/lib/objects.tsuseFillObject— mirrors the adjacentuseClearObjectexactly (schema,useMutation,onSettledinvalidate offullKey,onErrortoast via the sharedobjectErrorMessage). Comment explains the invalidate-vs-splice choice.FillLayouttype is the right place for it (consumed by the page and the control).web/src/pages/GardenEditorPage.tsx—FillControlis defined in-file (only used byPlantPlacementToolshere),RecentPlantsgot its own file (reusable, no page coupling); the asymmetry is justified by usage.cn,useState,useFillObject,FillLayout,recentlyPlantedIdsimports all present and used.PlantPlacementToolsprops grow by 4 but stay flat and readable; the desktop and mobile call sites pass identical props, so the "two can't drift" invariant the comment claims actually holds.layoutfield name matches the REST contract (internal/api/ops.go:57Layout string \json:"layout"`); the agent usesmode`, but the agent isn't this code's caller. No naming drift introduced..slice(0, 8)cap inrecentPlantsis a single magic number, but it's local, obvious, and the strip isoverflow-x-auto; not worth a named constant.No dead/duplicated code, confusing names, or pattern violations material enough to flag.
⚡ Performance — No material issues found
I'll verify the performance-relevant parts of this change against the actual code.
The performance-relevant code is small and contained. I verified:
recentlyPlantedIds(plantings.ts:69-82) sorts a shallow copy ([...plantings]) then dedups — no mutation of the source, O(n log n) over the garden's plantings. It's wrapped inuseMemokeyed on[plantings, plantsById](GardenEditorPage.tsx:118-125), so it only recomputes when the/fulldata actually changes, not on every render. The.slice(0, 8)happens after the full sort, so the whole array is sorted even though only 8 ids are needed — but this is a per-garden plantings list (tens to low hundreds at most), so an O(n log n) sort vs. a partial-select is not a material difference. Not worth flagging.useFillObject(objects.ts) does one POST and oneinvalidateQuerieson/full, mirroring the existinguseClearObjectpattern — no N+1, no optimistic splice to recompute. Standard.FillControlholds only localuseStatefor the layout toggle; no per-render allocation of note.RecentPlantsreturnsnullearly when empty and otherwise maps a ≤8-element array. Trivial.No N+1, no unbounded growth, no blocking calls on hot paths, no avoidable quadratic behavior, no missing pagination on a list that could grow (capped at 8 for display; the underlying sort is bounded by the garden's plantings).
VERDICT: No material issues found.
🧯 Error handling & edge cases — No material issues found
I've examined the diff and the surrounding code through the error-handling/edge-case lens. Let me summarize what I verified.
Verified items (all clean):
recentlyPlantedIds(web/src/lib/plantings.ts:69): operates on a spread copy ([...plantings].sort), so it doesn't mutate the caller's array. Empty input → empty result.plantedAt ?? ''makeslocaleComparesafe for null dates (undated sort last), and theb.id - a.idtie-break is total so the sort is deterministic. The three unit tests cover empty, dup-plant, same-day tie, and undated-last.recentPlantsmemo (GardenEditorPage.tsx:118):plantsById.get(id)can returnundefinedfor a plant no longer in the catalog; the type-guard filter(p): p is Plant => !!pdrops those silently rather than rendering a chip with a null plant. Resolves against the livefull.data.plants, which (perListReferencedPlantsininternal/store/plants.go:36) always contains every plant referenced by the active plops, so the filter is defensive rather than load-bearing.useFillObject(objects.ts:343): no optimistic cache update, so no rollback path is required.onErrortoasts viaobjectErrorMessage(ApiError message or fallback); a Zod parse failure of the response surfaces as the mutation error and hits the sameonError.onSettledinvalidates/fullon success and failure. MirrorsuseClearObjectexactly.fillBed(GardenEditorPage.tsx:414): guards!canEdit || focusedObject == null || armedPlant == nullbeforemutate.FillControlonly renders whenarmedPlantis truthy andPlantPlacementToolsonly undercanEdit && focusedObject.plantable, so the guards are belt-and-braces as commented. A race wherefocusedObjectbecomes non-plantable between render and click would be rejected server-side (fillLoadedcheckso.Plantable) → 400 → toast, not a crash.FillControl(GardenEditorPage.tsx:783): "Fill bed" button isdisabled={busy}, preventing a rapid double-fire that would enqueue two fills (the second would mostly no-op server-side anyway since covered spots are skipped, but the disable closes the door cleanly).canEditrequiresseasonYear === null, soPlantPlacementTools(and thusRecentPlants/FillControl) never render in a read-only past-season view.recentPlantsis still computed there (harmless wasted memo) but never displayed; no error path opens.defer/cleanup.Verdict: No material issues found
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
I reviewed this diff through the security lens. It is a frontend-only UI change (new React components
RecentPlants,FillControl, and hookuseFillObject) that surfaces two already-existing backend capabilities: the recent-planted strip and the bed-fill control both call endpoints that have been in place since #77.Findings checked and ruled out:
POST /objects/:id/filland the existing plant-placement APIs are behindh.requireAuth(). The service layer checksroleEditorinFillRegion/FillNamedRegion(confirmed ininternal/service/ops.go:179). API-level tests already enforce 403 for viewers and 404 for strangers (internal/api/ops_test.go:233). The new frontend code does not bypass any of this.objectIdandplantIdare numeric path/body parameters handled by the same typedapi.postwrapper used throughout the app. No SQL/command/template injection path is introduced.PlantIconandRecentPlantsrenderplant.nameandplant.icon(user-provided strings) in standard JSX text nodes and CSS properties. There is nodangerouslySetInnerHTML, no unescaped interpolation intohref/src/style, and thetitleattribute is safely interpolated. ThePlantIconcomponent appliescolorviastyle={{ backgroundColor: \color-mix(...)` }}`, which is a CSS color value; while this is technically user-controlled, it is already the established pattern used across the app and is not newly introduced here.FillLayouttype ('clump' | 'grid') is constrained at the TypeScript level, and the backend endpoint already validates thelayoutstring against known values and rejects unknowns withErrInvalidInput(internal/service/ops.go:196).Nothing in this diff materially affects the security posture.
🎯 Correctness — No material issues found
No material issues found
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/pages/GardenEditorPage.tsx:723— Stale comment: thePlantPlacementToolsdocstring still says(seed tray + Done + Clear), but the component now also rendersRecentPlantsandFillControl. Misleading comments rot faster than missing ones. Update it to mention the recent strip and fill control.web/src/pages/GardenEditorPage.tsx:121— Magic number8in.slice(0, 8)with no explanation. The cap is presumably “fits in the mobile strip”, but that’s not obvious from the code. Extract a named constant (e.g.,const MAX_RECENT_PLANTS = 8) at module level so the intent is obvious and the limit is in one place.web/src/pages/GardenEditorPage.tsx:783—FillControlis a standalone, reusable UI component (≈25 lines) defined inside an already-large page file (868 lines). The PR correctly extractedRecentPlantsto its own file;FillControlshould follow the same pattern and live inweb/src/editor/FillControl.tsxto keep the page file from growing and to match the codebase’s component-extraction convention.⚡ Performance — Minor issues
Verdict: Minor issues
web/src/lib/plantings.ts:69—recentlyPlantedIdscopies and sorts the entireplantingsarray (O(n log n)) even though callers only need the top 8 unique plant IDs. After a large "rows" fill, a garden can have hundreds or thousands of plops; every subsequent/fullrefetch (triggered by any mutation) re-sorts the full list to extract at most 8 items. A single-pass scan tracking the most recent planting perplantId(O(n)), followed by sorting only the unique entries, would scale linearly with plantings instead of super-linearly.Verified by reading
plantings.ts:69-82andGardenEditorPage.tsx:118-125. TherecentPlantsuseMemocallsrecentlyPlantedIds(plantings).slice(0, 8), confirming only the top 8 are consumed.🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the PR through the error handling & edge cases lens and found the code well-guarded:
recentlyPlantedIdshandles empty arrays, nullplantedAtvalues (sorting them last via the reversedlocaleComparetrick — verified by tracing the comparator logic), and deduplicates correctly. TherecentPlantsderivation filters out any plants missing from the catalog gracefully.fillBedhas belt-and-braces guards (!canEdit || focusedObject == null || armedPlant == null) that protect against race conditions where focus or arming changes between render and click. ThecanEditcheck also implicitly blocks all placement tools in season view (past seasons are read-only).useFillObjectuses TanStack Query'sisPendingto disable the fill button, preventing concurrent requests. Errors are toasted, and the invalidation happensonSettledso the cache refreshes regardless of success or failure.FillControlstate resets to'grid'when unmounted (i.e., when the plant is disarmed), which is the correct default behavior.plantingsin a past season includes removed plops (the Go side widens the query for historical views),canEditisfalsein season view, soPlantPlacementTools— and thus the Recent strip and fill controls — are never rendered. No leak of edit actions into read-only mode.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.