Plants mode: recent-planted quick strip + clump/rows fill (#100) #112

Merged
steve merged 2 commits from feat/plants-mode-recent into main 2026-07-22 05:58:39 +00:00
Owner

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.ai/code/session_01H3zbym8Doka2d7D48maSgZ

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
steve added 1 commit 2026-07-22 05:49:42 +00:00
Plants mode: recent-planted quick strip + clump/rows fill (#100)
Build image / build-and-push (push) Successful in 10s
Gadfly review (reusable) / review (pull_request) Successful in 9m44s
Adversarial Review (Gadfly) / review (pull_request) Successful in 9m44s
97c8a36bac
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

🪰 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

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>
steve added 1 commit 2026-07-22 05:58:08 +00:00
Address review: shared PlantChip + named recent-plants cap
Build image / build-and-push (push) Successful in 17s
7297138630
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 main 2026-07-22 05:58:39 +00:00
steve deleted branch feat/plants-mode-recent 2026-07-22 05:58:39 +00:00
gitea-actions bot reviewed 2026-07-22 05:59:26 +00:00
gitea-actions bot left a comment

🪰 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>
@@ -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-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>
@@ -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:69recentlyPlantedIds 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>
@@ -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) 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>
@@ -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:783FillControl 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>

🪰 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
2 single-model findings (lower confidence)
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
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

VERDICT: No material issues found

Reviewed through the security lens (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.

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

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 } 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-82recentlyPlantedIds 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-125recentPlants 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 gridrows, 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.

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 — 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-24aria-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:

  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 contractuseFillObject 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 compatibilityRecentPlants.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 invalidationuseFillObject 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.
  • 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.

🧹 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.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.tsxFillControl 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.

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

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 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:783FillControl 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:69recentlyPlantedIds 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.

🧯 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>
Sign in to join this conversation.