Seed-packet scan UI: camera/upload → proposal → confirm (#102) #119

Merged
steve merged 2 commits from feat/seed-packet-scan-ui into main 2026-07-22 17:07:26 +00:00
Owner

Closes #102. The last open child of epic #96.

The seed-packet capture backend has been live since #94POST /seed-lots/scan (multipart image → a PacketProposal) and POST /seed-lots/from-packet (confirm → plant + lot), with /capabilities reporting vision — but there was no UI, so the feature Steve specifically asked for ("take a picture of a packet and it auto-creates") was unreachable. This adds the mobile-first UI.

What's here

  • Entry point in the Plants catalog ("Scan a packet"), shown only where capabilities.vision is on — never a dead button on an instance with no vision model. vision is added to the capabilities schema (both flags now default(false), so a partial/older response hides a feature rather than failing the parse).
  • ScanPacketModal — two phases in one dialog:
    • Capture: a file input with accept="image/*" capture="environment" so a phone opens the camera directly; also picks an existing photo (HEIC handled server-side). Shows a scanning state during the multi-second vision call.
    • Review: the extracted fields read-only for context, then an editable confirm — pick a ranked existing candidate (with its match reason) or create a new variety prefilled from the packet. Nothing commits blind, honoring the backend's never-auto-create design. Handles the 503 VISION_DISABLED / unreadable / too-large cases via the server message.
  • lib/seedPacket.ts — zod schemas mirroring the backend, the scan + confirm mutations, and pure prefill helpers (newPlantDefaults / lotDefaults), unit-tested where the bugs hide (unknown category → safe fallback, missing spacing → default, seed-count → unit).
  • api.ts learns to send FormData as multipart (no JSON content-type) for the upload, via a new api.postForm.

Frontend only; no backend or route change.

Testing

tsc clean, 108 web tests green (7 new for the schemas + prefill), npm run build clean (no chunk-size warning; vendor chunk hash unchanged, so no chunking regression).

The live camera → vision → proposal path is model-gated and unreachable locally (same as the assistant UI). Its prefill logic and the confirm half are covered by tests; end-to-end camera verification is pending a vision-configured instance.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

Closes #102. The last open child of epic #96. The seed-packet capture **backend** has been live since #94 — `POST /seed-lots/scan` (multipart image → a `PacketProposal`) and `POST /seed-lots/from-packet` (confirm → plant + lot), with `/capabilities` reporting `vision` — but there was no UI, so the feature Steve specifically asked for ("take a picture of a packet and it auto-creates") was unreachable. This adds the mobile-first UI. ### What's here - **Entry point** in the Plants catalog ("Scan a packet"), shown only where `capabilities.vision` is on — never a dead button on an instance with no vision model. `vision` is added to the capabilities schema (both flags now `default(false)`, so a partial/older response hides a feature rather than failing the parse). - **`ScanPacketModal`** — two phases in one dialog: - *Capture*: a file input with `accept="image/*" capture="environment"` so a phone opens the camera directly; also picks an existing photo (HEIC handled server-side). Shows a scanning state during the multi-second vision call. - *Review*: the extracted fields read-only for context, then an **editable** confirm — pick a ranked existing candidate (with its match reason) or create a new variety prefilled from the packet. Nothing commits blind, honoring the backend's never-auto-create design. Handles the 503 `VISION_DISABLED` / unreadable / too-large cases via the server message. - **`lib/seedPacket.ts`** — zod schemas mirroring the backend, the scan + confirm mutations, and pure prefill helpers (`newPlantDefaults` / `lotDefaults`), **unit-tested** where the bugs hide (unknown category → safe fallback, missing spacing → default, seed-count → unit). - **`api.ts`** learns to send `FormData` as multipart (no JSON content-type) for the upload, via a new `api.postForm`. Frontend only; no backend or route change. ### Testing `tsc` clean, 108 web tests green (7 new for the schemas + prefill), `npm run build` clean (no chunk-size warning; vendor chunk hash unchanged, so no chunking regression). The live **camera → vision → proposal** path is model-gated and unreachable locally (same as the assistant UI). Its prefill logic and the confirm half are covered by tests; end-to-end camera verification is pending a vision-configured instance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve added 1 commit 2026-07-22 16:52:45 +00:00
Seed-packet scan UI: camera/upload → proposal → confirm (#102)
Build image / build-and-push (push) Successful in 23s
Gadfly review (reusable) / review (pull_request) Successful in 10m32s
Adversarial Review (Gadfly) / review (pull_request) Successful in 10m33s
e0d78b891b
The seed-packet capture backend has been live since #94 (POST /seed-lots/scan
and /seed-lots/from-packet, with /capabilities reporting `vision`), but there was
no UI, so the feature Steve asked for — "take a picture of a packet and it
auto-creates" — was unreachable. This adds the mobile-first UI.

- Entry point in the Plants catalog ("Scan a packet"), shown only where
  `capabilities.vision` is on — so it's never a dead button on an instance with
  no vision model. Adds `vision` to the capabilities schema (both flags now
  default false, so a partial response hides a feature rather than failing parse).
- ScanPacketModal: two phases in one dialog. Capture uses a file input with
  `accept="image/*" capture="environment"` so a phone opens the camera directly
  (also picks an existing photo; HEIC is handled server-side). Review shows the
  extracted fields read-only for context, then an editable confirm — pick a ranked
  existing candidate (with its match reason) or create a new variety prefilled from
  the packet. Nothing commits blind, which is the whole point of the backend's
  never-auto-create design.
- New data layer (lib/seedPacket.ts): zod schemas mirroring the backend
  (SeedPacket / PacketProposal / PacketResult), the scan + confirm mutations, and
  the pure prefill helpers (newPlantDefaults / lotDefaults) — unit-tested, since
  the prefill mapping (unknown category, missing spacing, seed-count→unit) is
  where the bugs hide.
- api.ts learns to send FormData as multipart (no JSON content-type) for the
  scan upload, via a new api.postForm.

Frontend only; no backend or route change. Last open child of epic #96.

Live camera→vision→proposal verification needs a vision-configured instance
(the flow is model-gated and unreachable locally, same as the assistant);
the confirm half and all prefill logic are covered by tests.

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 17:03:19Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

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

opencode/glm-5.2:cloud · opencode — done

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

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

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — 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 17:03:19Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Blocking issues found #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-22 17:03:20 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 10 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** — 10 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +23,4 @@
import { LOT_UNITS, type LotUnit } from '@/lib/seedLots'
import { cmFromSpacing, spacingFromCm, spacingUnitLabel, type UnitPref } from '@/lib/units'
const categoryOptions = PLANT_CATEGORIES.map((c) => ({ value: c, label: CATEGORY_LABELS[c] }))

🟡 categoryOptions duplicated from PlantFormModal

maintainability · flagged by 1 model

  • web/src/components/plants/ScanPacketModal.tsx:26categoryOptions is identical to the one in PlantFormModal.tsx:25. It should be exported once from @/lib/plants (e.g. export const categoryOptions = …) and imported by both modals.

🪰 Gadfly · advisory

🟡 **categoryOptions duplicated from PlantFormModal** _maintainability · flagged by 1 model_ - **`web/src/components/plants/ScanPacketModal.tsx:26`** — `categoryOptions` is identical to the one in `PlantFormModal.tsx:25`. It should be exported once from `@/lib/plants` (e.g. `export const categoryOptions = …`) and imported by both modals. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +44,4 @@
const create = useCreateFromPacket()
const fileInput = useRef<HTMLInputElement>(null)
const [proposal, setProposal] = useState<PacketProposal | null>(null)

14 useState slots in one ~330-line component; a reducer/single review-state object would reduce the setter wall

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **14 useState slots in one ~330-line component; a reducer/single review-state object would reduce the setter wall** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +85,4 @@
setCategory(plant.category)
setSpacing(String(spacingFromCm(plant.spacingCm, unit)))
setDays(plant.daysToMaturity != null ? String(plant.daysToMaturity) : '')
setVendor(lot.vendor)

🔴 Cost field persists across rescans because setCost('') is missing in onFile success handler

correctness · flagged by 1 model

  • web/src/components/plants/ScanPacketModal.tsx:88-95 — The cost form state is never reset when a new packet is scanned. onFile sets name, category, spacing, days, vendor, quantity, lotUnit, sku, lotCode, and packedForYear from the new proposal, but setCost('') is missing. Consequently, if a user enters a cost for one packet, clicks "Rescan", and scans a different packet, the old cost value silently persists and will be submitted with the new lot. Fix: add `se…

🪰 Gadfly · advisory

🔴 **Cost field persists across rescans because setCost('') is missing in onFile success handler** _correctness · flagged by 1 model_ * **`web/src/components/plants/ScanPacketModal.tsx:88-95`** — The `cost` form state is never reset when a new packet is scanned. `onFile` sets `name`, `category`, `spacing`, `days`, `vendor`, `quantity`, `lotUnit`, `sku`, `lotCode`, and `packedForYear` from the new proposal, but `setCost('')` is missing. Consequently, if a user enters a cost for one packet, clicks "Rescan", and scans a different packet, the old cost value silently persists and will be submitted with the new lot. **Fix:** add `se… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +102,4 @@
if (!proposal) return
setError(null)
// Lot validation mirrors SeedLotModal so the two paths accept the same things.

🟠 Lot validation (qty/year/cost) hand-copied from SeedLotModal with no shared helper; comment claims mirroring but nothing enforces it

maintainability · flagged by 4 models

  • web/src/components/plants/ScanPacketModal.tsx:105-128 — duplicated lot validation. The onConfirm qty / packed-for-year / cost validation blocks are character-for-character copies of SeedLotModal.tsx:62-88, and the comment at line 105 explicitly says "Lot validation mirrors SeedLotModal so the two paths accept the same things." Verified by reading both files: the Quantity must be a number, or left blank. string, costCents = Math.round(c * 100), and the 1900/2200 year range a…

🪰 Gadfly · advisory

🟠 **Lot validation (qty/year/cost) hand-copied from SeedLotModal with no shared helper; comment claims mirroring but nothing enforces it** _maintainability · flagged by 4 models_ - **`web/src/components/plants/ScanPacketModal.tsx:105-128` — duplicated lot validation.** The `onConfirm` qty / packed-for-year / cost validation blocks are character-for-character copies of `SeedLotModal.tsx:62-88`, and the comment at line 105 explicitly says *"Lot validation mirrors SeedLotModal so the two paths accept the same things."* Verified by reading both files: the `Quantity must be a number, or left blank.` string, `costCents = Math.round(c * 100)`, and the `1900`/`2200` year range a… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +127,4 @@
costCents = Math.round(c * 100)
}
const lot = {

🟡 Hand-built lot literal re-spells SeedLotFields instead of spreading lotDefaults + overrides

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Hand-built `lot` literal re-spells SeedLotFields instead of spreading lotDefaults + overrides** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +140,4 @@
germinationPct: null,
notes: '',
}

🟠 parseFloat(spacing) is less strict than Number() used for other fields, allowing malformed spacing input

correctness · flagged by 1 model

  • web/src/components/plants/ScanPacketModal.tsx:143 — Spacing validation uses parseFloat(spacing), while quantity and days use the stricter Number() (lines 106, 149). parseFloat accepts trailing garbage (e.g. "15abc"15), so malformed spacing input can slip through where other fields would be rejected. Since the input is type="number" this is harder to trigger in practice, but the inconsistency weakens validation. Fix: replace parseFloat(spacing) with `Number(spacin…

🪰 Gadfly · advisory

🟠 **parseFloat(spacing) is less strict than Number() used for other fields, allowing malformed spacing input** _correctness · flagged by 1 model_ * **`web/src/components/plants/ScanPacketModal.tsx:143`** — Spacing validation uses `parseFloat(spacing)`, while `quantity` and `days` use the stricter `Number()` (lines 106, 149). `parseFloat` accepts trailing garbage (e.g. `"15abc"` → `15`), so malformed spacing input can slip through where other fields would be rejected. Since the input is `type="number"` this is harder to trigger in practice, but the inconsistency weakens validation. **Fix:** replace `parseFloat(spacing)` with `Number(spacin… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +148,4 @@
setError('Name the new variety, or pick an existing plant above.')
return
}
const spacingCm = cmFromSpacing(parseFloat(spacing), unit)

🟡 New-plant spacing/days validation copied verbatim from PlantFormModal; same duplication pattern as the lot validation

correctness, maintainability · flagged by 2 models

  • web/src/components/plants/ScanPacketModal.tsx:151-163 — duplicated new-plant field validation. The selection === 'new' branch re-implements spacing / days-to-maturity validation that PlantFormModal.tsx:86-100 already has (verified: the spacing < 1 check with the same Spacing must be at least 1 ${unitLabel}. message and "Days to maturity must be a whole number of days" are copied verbatim). Less severe than the lot duplication because the field sets aren't identical (the packet fo…

🪰 Gadfly · advisory

🟡 **New-plant spacing/days validation copied verbatim from PlantFormModal; same duplication pattern as the lot validation** _correctness, maintainability · flagged by 2 models_ - **`web/src/components/plants/ScanPacketModal.tsx:151-163` — duplicated new-plant field validation.** The `selection === 'new'` branch re-implements spacing / days-to-maturity validation that `PlantFormModal.tsx:86-100` already has (verified: the spacing `< 1` check with the same `Spacing must be at least 1 ${unitLabel}.` message and "Days to maturity must be a whole number of days" are copied verbatim). Less severe than the lot duplication because the field sets aren't identical (the packet fo… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +188,4 @@
}
return (
<Modal title="Scan a seed packet" onClose={onClose} busy={busy}>

🟡 No cancel/timeout for the scan mutation; Modal blocks all close paths while busy, trapping the user during a slow/stuck vision call

error-handling · flagged by 1 model

  • web/src/components/plants/ScanPacketModal.tsx:191 (with Modal's blocking logic at web/src/components/ui/Modal.tsx:47,94): while a scan is in flight, there is no way to cancel or bail out. Modal disables Escape (Modal.tsx:47, gated on !busyRef.current) and backdrop-click-to-close (Modal.tsx:94, gated on !busy) whenever busy is true, and ScanPacketModal's own capture-phase Cancel button is disabled={busy} (ScanPacketModal.tsx:227). Confirmed `busy = scan.isPending || crea…

🪰 Gadfly · advisory

🟡 **No cancel/timeout for the scan mutation; Modal blocks all close paths while busy, trapping the user during a slow/stuck vision call** _error-handling · flagged by 1 model_ - `web/src/components/plants/ScanPacketModal.tsx:191` (with `Modal`'s blocking logic at `web/src/components/ui/Modal.tsx:47,94`): while a scan is in flight, there is no way to cancel or bail out. `Modal` disables Escape (`Modal.tsx:47`, gated on `!busyRef.current`) and backdrop-click-to-close (`Modal.tsx:94`, gated on `!busy`) whenever `busy` is true, and `ScanPacketModal`'s own capture-phase Cancel button is `disabled={busy}` (`ScanPacketModal.tsx:227`). Confirmed `busy = scan.isPending || crea… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +360,4 @@
{error && <Alert>{error}</Alert>}
<div className="mt-1 flex justify-between gap-2">
<Button type="button" variant="ghost" onClick={() => setProposal(null)} disabled={busy}>

🔴 Stale review-phase error persists after clicking Rescan

error-handling · flagged by 2 models

🪰 Gadfly · advisory

🔴 **Stale review-phase error persists after clicking Rescan** _error-handling · flagged by 2 models_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +93,4 @@
// A packet has no icon or color, so a new variety created from one gets these
// placeholders; the user can set a real icon/color later from the plant card.
export const PACKET_PLANT_COLOR = '#4a7c3f'

🟡 PACKET_PLANT_COLOR/PACKET_PLANT_ICON duplicate PlantFormModal's DEFAULT_COLOR/DEFAULT_ICON under new names; same value coupled by intent, decoupled in code

maintainability · flagged by 1 model

  • web/src/lib/seedPacket.ts:96-97 — duplicated default color/icon constants. PACKET_PLANT_COLOR = '#4a7c3f' and PACKET_PLANT_ICON = '🌱' are the same values as DEFAULT_COLOR/DEFAULT_ICON in PlantFormModal.tsx:22-23 (verified by grep — both files declare these literals, no shared export). They're renamed and re-declared rather than imported, so the two are coupled by intent ("a new plant with no icon gets this placeholder") but decoupled in code — if the manual form's default chang…

🪰 Gadfly · advisory

🟡 **PACKET_PLANT_COLOR/PACKET_PLANT_ICON duplicate PlantFormModal's DEFAULT_COLOR/DEFAULT_ICON under new names; same value coupled by intent, decoupled in code** _maintainability · flagged by 1 model_ - **`web/src/lib/seedPacket.ts:96-97` — duplicated default color/icon constants.** `PACKET_PLANT_COLOR = '#4a7c3f'` and `PACKET_PLANT_ICON = '🌱'` are the same values as `DEFAULT_COLOR`/`DEFAULT_ICON` in `PlantFormModal.tsx:22-23` (verified by grep — both files declare these literals, no shared export). They're renamed and re-declared rather than imported, so the two are coupled by intent ("a new plant with no icon gets this placeholder") but decoupled in code — if the manual form's default chang… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Blocking issues found · 10 findings (3 with multi-model agreement)

Finding Where Models Lens
🟠 Lot validation (qty/year/cost) hand-copied from SeedLotModal with no shared helper; comment claims mirroring but nothing enforces it web/src/components/plants/ScanPacketModal.tsx:105 4/5 maintainability
🔴 Stale review-phase error persists after clicking Rescan web/src/components/plants/ScanPacketModal.tsx:363 2/5 error-handling
🟡 New-plant spacing/days validation copied verbatim from PlantFormModal; same duplication pattern as the lot validation web/src/components/plants/ScanPacketModal.tsx:151 2/5 correctness, maintainability
7 single-model findings (lower confidence)
Finding Where Model Lens
🔴 Cost field persists across rescans because setCost('') is missing in onFile success handler web/src/components/plants/ScanPacketModal.tsx:88 kimi-k2.6:cloud correctness
🟠 parseFloat(spacing) is less strict than Number() used for other fields, allowing malformed spacing input web/src/components/plants/ScanPacketModal.tsx:143 kimi-k2.6:cloud correctness
🟡 categoryOptions duplicated from PlantFormModal web/src/components/plants/ScanPacketModal.tsx:26 opencode/kimi-k2.6:cloud maintainability
🟡 Hand-built 'lot' literal re-spells SeedLotFields instead of spreading lotDefaults + overrides web/src/components/plants/ScanPacketModal.tsx:130 glm-5.2:cloud maintainability
🟡 No cancel/timeout for the scan mutation; Modal blocks all close paths while busy, trapping the user during a slow/stuck vision call web/src/components/plants/ScanPacketModal.tsx:191 claude-code/sonnet error-handling
🟡 PACKET_PLANT_COLOR/PACKET_PLANT_ICON duplicate PlantFormModal's DEFAULT_COLOR/DEFAULT_ICON under new names; same value coupled by intent, decoupled in code web/src/lib/seedPacket.ts:96 opencode/glm-5.2:cloud maintainability
14 useState slots in one ~330-line component; a reducer/single review-state object would reduce the setter wall web/src/components/plants/ScanPacketModal.tsx:47 glm-5.2:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

VERDICT: No material issues found

I reviewed this diff specifically through a security lens (authn/authz, injection, unsafe deserialization, secret leakage, untrusted-input handling), verifying against the checked-out repo rather than the diff text alone:

  • web/src/lib/api.ts (apiFetch/postForm) — confirmed the FormData branch never sets a manual content-type, so the browser generates the multipart boundary correctly; the JSON branch is unaffected. credentials: 'same-origin' is preserved, so the session cookie handling didn't change. No injection surface here — path values passed to api.postForm/api.post are static string literals (/seed-lots/scan, /seed-lots/from-packet), not built from user input.
  • ScanPacketModal.tsx — all model-extracted / candidate data (p.species, c.reason, c.plant.name, etc.) is rendered as plain JSX text content, no dangerouslySetInnerHTML or similar sink. selection/plantId is constrained to number | 'new' and the numeric branch only ever comes from c.plant.id in the server-returned candidate list (via the radio inputs) — not free-typed, so it can't be used to reference an arbitrary/unauthorized plant ID through client tampering beyond what the server itself already returned for this user.
  • seedPacket.ts — all response data passes through zod .parse() with .default() fallbacks before use; an unexpected/malicious category string is defensively re-validated client-side (isPlantCategory) in addition to the server-side check the comment notes, so a bad suggestedCategory can't leak an invalid enum value into the create payload.
  • internal/api/seed_packet.go (unchanged, confirmed via grep) — the existing backend already enforces auth (401 on no session), payload size limits (413), and vision-disabled gating (503), and this PR is frontend-only, so no backend authz/validation surface was touched or bypassed. The client-side capabilities.vision check is purely a UX gate (hide vs. show a button); it grants no access itself since the server independently gates the vision call.
  • Capability schema change (agent/vision now .default(false)) fails safe: a partial/malformed /capabilities response hides features rather than exposing them, which is the correct direction.

No taint path from the untrusted image/model output to a dangerous sink, no new unauthenticated endpoint, no secret handling in this diff.

🎯 Correctness — No material issues found

Modal already guards busy-close, so no issue there. Confirmed the diff is well-aligned with the backend contract and existing form patterns. I've verified schema field names, units math, and JSON shapes against the Go backend and sibling components — nothing correctness-wise stands out.

VERDICT: No material issues found

Verification performed:

  • Cross-checked seedPacketSchema/packetProposalSchema/packetResultSchema (web/src/lib/seedPacket.ts) field-by-field against vision.SeedPacket, service.PacketProposal/PacketConfirm/PacketResult, and the fromPacketRequest/plantCreateRequest/seedLotFields JSON tags in internal/api/{seed_packet,plants,seed_lots}.go — names and optionality match.
  • Re-derived the unit math: cmFromSpacing/spacingFromCm (web/src/lib/units.ts:141-152) and DEFAULT_SPACING_CM = 30 match the existing PlantFormModal conventions exactly (same default, same spacingCm < 1 floor, same round-trip via roundCm).
  • Verified Selection = number | 'new' against plantSchema.id: z.number() (web/src/lib/plants.ts) — no type mismatch, and the ?? 'new' (not ||) correctly treats a candidate id of 0 as valid.
  • Verified plantId/newPlant are mutually exclusive when serialized (JSON.stringify drops undefined), matching the backend's hasID == hasNew XOR check in CreateFromPacket (internal/service/seed_packet.go:189-192).
  • Confirmed isPlantCategory/PLANT_CATEGORIES matches the backend's plantCategories set used by validCategory (vegetable/herb/flower/fruit/tree_shrub/cover) — an unknown/model-hallucinated category can't slip through.
  • Confirmed apiFetch's new FormData branch omits the JSON content-type header (letting the browser set the multipart boundary) and that the field name image matches c.FormFile("image") server-side.
  • Confirmed Modal's busy flag blocks Escape/backdrop close during the scan/create mutations, so no race with unmounting mid-request.
🧹 Code cleanliness & maintainability — Minor issues

Confirmed line-by-line: lines 106-127 in ScanPacketModal.tsx match SeedLotModal.tsx:62-88 exactly (same conditions, same error strings, same 1900/2200 bounds), and lines 151-164 match PlantFormModal.tsx:86-100 exactly (same spacing/days parsing and error strings). The draft's finding is accurate.

Verdict: Minor issues

  • web/src/components/plants/ScanPacketModal.tsx:106-127 duplicates the quantity/packedForYear/cost validation verbatim from web/src/components/plants/SeedLotModal.tsx:62-88 (identical conditions, error strings, and the 1900/2200 year bounds — confirmed by direct comparison of both files), and ScanPacketModal.tsx:151-164 separately duplicates the spacing/days-to-maturity validation from web/src/components/plants/PlantFormModal.tsx:86-100. This one new file now carries a second copy of both other modals' parsing logic, so there are three independent places encoding "how to parse a packed-for year" / "cost" / "days to maturity" — a future change to those bounds or messages must be applied in three places to stay consistent. Extracting a shared helper (e.g. parseLotFields(...) returning {value} | {error}) would remove the duplication and the drift risk.
    • The codebase already tolerates some inline duplication between SeedLotModal and PlantFormModal, so this isn't a new convention violation, but this PR compounds it with a third and fourth copy in one file.
Performance — No material issues found

VERDICT: No material issues found

Reviewed through the Performance lens: this is a small, self-contained frontend feature (a scan modal, a schema/prefill helper module, and a postForm addition to the API client). Checked for the usual suspects:

  • No loops over unbounded data — proposal.candidates and the various form arrays (PLANT_CATEGORIES, LOT_UNITS) are small, fixed-size, module-scope constants (verified in web/src/lib/plants.ts:10 and web/src/lib/seedLots.ts:13-20), not recomputed per render.
  • The scan (useScanPacket) is correctly a one-shot, uncached mutation — appropriate since each photo is unique; no risk of stale-cache growth.
  • useCreateFromPacket's onSuccess invalidates exactly plantsKey and seedLotsKey (web/src/lib/seedPacket.ts:66-69) — matches the two catalogs actually affected, no over-invalidation cascading unrelated queries.
  • The multipart path in api.ts (web/src/lib/api.ts:98-101) passes the FormData through as-is with no added serialization/copy step — no unnecessary buffering of the image.
  • newPlantDefaults(proposal) is called twice across the component lifecycle (once for prefill in onFile, once again in onConfirm), but it's a cheap plain-object construction on a small, already-parsed proposal — not a measurable cost.
  • No new polling, no added blocking calls on any hot path; the only slow call (vision scan) is already gated behind an explicit user action and a pending/spinner state, not something that runs repeatedly or in a loop.

Nothing here rises to a real efficiency regression. This is frontend-only, one-shot-interaction code with no data-scale exposure.

🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues

  • web/src/components/plants/ScanPacketModal.tsx:191 (with Modal's blocking logic at web/src/components/ui/Modal.tsx:47,94): while a scan is in flight, there is no way to cancel or bail out. Modal disables Escape (Modal.tsx:47, gated on !busyRef.current) and backdrop-click-to-close (Modal.tsx:94, gated on !busy) whenever busy is true, and ScanPacketModal's own capture-phase Cancel button is disabled={busy} (ScanPacketModal.tsx:227). Confirmed busy = scan.isPending || create.isPending (ScanPacketModal.tsx:67). Confirmed server-side the scan endpoint deliberately extends deadlines well past instant-CRUD expectations (internal/api/seed_packet.go:30,37: scanReadTimeout = 60s, scanWriteTimeout = 120s), because a slow phone upload plus a live vision call can legitimately run long — the modal's own copy only promises "this can take a few seconds" (ScanPacketModal.tsx:216). Confirmed neither useScanPacket's mutationFn (web/src/lib/seedPacket.ts:70-78, async (file: File), no signal threaded through) nor apiFetch is ever invoked with an AbortSignal for this call, even though RequestOptions.signal exists and is honored (web/src/lib/api.ts:50,106) — it's simply never passed here. Net effect: a hung or slow vision call leaves the user with no in-app way to close the dialog short of reloading the page, which is a materially worse failure mode than the same "block close while busy" convention applied to sub-second CRUD modals like SeedLotModal/PlantFormModal. Suggested fix: wire an AbortController into the scan mutation and let Cancel/Escape abort it, or allow closing after a shorter client-side timeout with a clear message.
glm-5.2:cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

Verdict: No material issues found

Through the security lens I traced the tainted-data path in this PR: server-vision output is the untrusted input, and it flows through seedPacketSchema/packetProposalSchema (zod parse) before being rendered or sent back. Verified the key points:

  • No injection / no dangerouslySetInnerHTML: grep confirms none in web/src. The extracted packet fields (species, variety, reason, etc.) are rendered as React text children ({c.reason}, <dd>{v}</dd>, TextField value=...), so they're auto-escaped — no XSS sink. Verified Alert.tsx renders children as plain ReactNode, not raw HTML.
  • Untrusted model output is schema-validated before use: useScanPacket parses via packetProposalSchema (and nested plantSchema/seedPacketSchema) before the data reaches state; useCreateFromPacket parses the result via packetResultSchema. Unknown category is explicitly neutralized to 'vegetable' via isPlantCategory rather than passed through — a malformed/attacker-influenced model response can't smuggle an arbitrary category into PlantInput.
  • Multipart upload is sent with no manual content-type: api.postForm lets the browser set the multipart boundary; the diff correctly omits the JSON content-type for FormData (api.ts:99-112). No header injection / boundary-clobber path.
  • Confirm body is built from user-edited, validated fields: client-side validation (qty, year range, costCents, spacingCm) mirrors the existing lot form; the server is the real authority per the backend design, so these are UX, not a security boundary — but no unvalidated server string is reflected back unsanitized.
  • CSRF/auth: upload uses credentials: 'same-origin' (session cookie) same as every other call; no new auth surface introduced. Gating the entry point on capabilities.vision prevents a dead/404-ing button but isn't a security control, and the 503 path is handled.

Nothing in my lane materially wrong.

🎯 Correctness — Minor issues

Verdict: Minor issues found

  • web/src/components/plants/ScanPacketModal.tsx:153 — spacing validation floor is 1 cm but the error message quotes the display unit. The guard is spacingCm < 1 where spacingCm = cmFromSpacing(parseFloat(spacing), unit) (cmFromSpacing returns value * CM_PER_INCH for imperial, CM_PER_INCH = 2.54, at web/src/lib/units.ts:191-203,7), so the real floor is 1 cm. The message reads `Spacing must be at least 1 ${unitLabel}.` and spacingUnitLabel returns in for imperial. In metric this is correct ("1 cm"); in imperial the message says "at least 1 in" while the actual floor is ~0.39 in (1 cm). A user entering 0.5 in (≈1.27 cm) passes despite being told the minimum is 1 in, and 0.4 in (≈1.0 cm) is rejected with a message claiming the floor is 1 in. Fix: state the threshold in cm universally, or compute a unit-specific floor (cmFromSpacing(1, unit) / spacingFromCm(1, unit)) and compare/display in the entry unit. (Verified against units.ts:191-203 and the <TextField min="1"> at line ~245, which has the same cm/in mismatch in imperial.)
🧹 Code cleanliness & maintainability — Minor issues

All findings verified against the actual source. Findings 1–3 confirmed; finding 4 contradicted by the code (the three constants are already consecutive — isPlantCategory follows them, it doesn't split them).

Review: 🧹 Code cleanliness & maintainability

VERDICT: Minor issues

  • ScanPacketModal.tsx:105–128 — duplicated lot-validation block. The quantity/year/cost parsing and the three error strings ('Quantity must be a number, or left blank.', 'Packed-for year should be a four-digit year.', 'Cost must be an amount, or left blank.') plus Math.round(c * 100) are copy-pasted from SeedLotModal.tsx:62–88. The comment on line 105 even admits it ("Lot validation mirrors SeedLotModal"). A fix to the year range or cost rounding in one modal won't reach the other. Suggested fix: extract a pure parseLotFields(...) helper returning { values, error } into lib/seedLots.ts and call it from both modals. Low churn, one source of truth.

  • ScanPacketModal.tsx:130–142 — hand-built lot object re-spells SeedLotFields. The literal sets every field of SeedLotFields (seedPacket.ts:57), hard-coding sourceUrl: '', purchasedAt: null, germinationPct: null, notes: '' to the exact values lotDefaults already returns (seedPacket.ts:131–147); the only deltas are the user-edited quantity/unit/costCents/packedForYear/sku/lotCode/vendor. lotDefaults is already imported and used in the prefill onSuccess, so { ...lotDefaults(proposal.packet), <edited overrides> } would remove the duplication and keep the two paths from diverging on which fields exist.

  • ScanPacketModal.tsx:47–64 — 14 useState slots in one ~330-line component. The component mixes capture, prefill, validation, and confirm in one body, and the prefill onSuccess (lines ~78–93) scatters newPlantDefaults/lotDefaults results across a wall of setters. Readable now, but a useReducer or single reviewState object for the review-phase fields would flatten the setter wall and make prefill a single assignment — a natural follow-up rather than something to force in this PR.

Performance — No material issues found

No material issues found

Reviewed through the performance lens: the new modal holds a single File in React state (transient, GC'd after the mutation), ReadFields builds a small array of rows from a handful of proposal fields (bounded, not in a hot path), the candidates list is rendered once (model-gated, tiny N), and the confirm invalidates exactly the two catalog query keys (plantsKey, seedLotsKey) — no fan-out or N+1. postForm correctly avoids JSON.stringify and lets the browser stream the multipart body. useScanPacket is uncached by design (one-shot per photo), so no unbounded growth. Nothing here is a material efficiency regression.

🧯 Error handling & edge cases — Minor issues

Confirmed. The finding is valid: Rescan handler at line 363 is () => setProposal(null) with no setError(null); the shared error state is rendered in both the capture phase (line 224) and review phase (line 360); a confirm failure sets it to "Could not save the packet." (line 186); and it only clears once a new file is picked (onFile at line 75). The draft cited line 284, but the actual Rescan button is at line 363 — correcting that.

Verdict: Minor issues

  • Stale error persists into the capture phase after "Rescan." ScanPacketModal.tsx:363 — the Rescan button's handler is () => setProposal(null) with no setError(null). If a confirm attempt failed (e.g. server rejected the lot), error holds "Could not save the packet." (set at ScanPacketModal.tsx:186). Clicking Rescan returns the user to the capture phase, which renders the same shared {error && <Alert>} block (ScanPacketModal.tsx:224), so the misleading save-failure message is shown over the "Take or choose a photo" UI. It only clears once the user actually picks a new file (because onFile calls setError(null) at ScanPacketModal.tsx:75). error is one shared state variable rendered in both phases. Suggested fix: clear error in the Rescan handler, e.g. onClick={() => { setProposal(null); setError(null) }}.

No other material error-handling issues: the scan onError correctly surfaces the server message, the confirm try/catch wraps only the awaited mutation (validation failures return rather than throw), void qc.invalidateQueries is the intended fire-and-forget, and the null/empty guards in ReadFields (p.spacingCm != null, p.seedCount != null) and lotDefaults (seedCount != null && seedCount > 0) are correct.

kimi-k2.6:cloud (ollama-cloud) — Blocking issues found

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

🔒 Security — No material issues found

No material issues found

I reviewed the seed-packet scan UI changes through the security lens. The file upload path, server response handling, form rendering, and toast notifications are all safe. Key observations:

  • File upload: api.postForm correctly omits the content-type header for FormData, letting the browser generate the multipart boundary. No path traversal or injection risk — the File object is passed as-is.
  • XSS prevention: Extracted packet data (species, variety, etc.) is rendered as React text nodes in ReadFields and form inputs — no dangerouslySetInnerHTML, no HTML injection surface. Toast and Alert components also render text, not HTML.
  • Response validation: Server responses from /seed-lots/scan and /seed-lots/from-packet are parsed through Zod schemas (packetProposalSchema, packetResultSchema), preventing malformed data from propagating.
  • No secret leakage: No hardcoded credentials, tokens, or API keys introduced.
  • Capability gating: The scan button is correctly hidden when capabilities.vision is false, preventing 404/503 exposure.
  • Client-side validation: Quantity, year, cost, spacing, and days are validated before submission, complementing server-side checks.

The changes are clean from a security perspective.

🎯 Correctness — Blocking issues found

Blocking issues found

  • web/src/components/plants/ScanPacketModal.tsx:88-95 — The cost form state is never reset when a new packet is scanned. onFile sets name, category, spacing, days, vendor, quantity, lotUnit, sku, lotCode, and packedForYear from the new proposal, but setCost('') is missing. Consequently, if a user enters a cost for one packet, clicks "Rescan", and scans a different packet, the old cost value silently persists and will be submitted with the new lot. Fix: add setCost('') in the onFile success handler.

  • web/src/components/plants/ScanPacketModal.tsx:143 — Spacing validation uses parseFloat(spacing), while quantity and days use the stricter Number() (lines 106, 149). parseFloat accepts trailing garbage (e.g. "15abc"15), so malformed spacing input can slip through where other fields would be rejected. Since the input is type="number" this is harder to trigger in practice, but the inconsistency weakens validation. Fix: replace parseFloat(spacing) with Number(spacing) for consistency.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

The code is well-structured and follows existing patterns. A few observations, none material:

  • ScanPacketModal is large (~400 lines) but is appropriately a single cohesive unit — a two-phase modal where the review form's fields are tightly coupled to the capture phase. Further splitting would add indirection without improving readability.
  • PACKET_PLANT_COLOR/PACKET_PLANT_ICON in seedPacket.ts duplicate the values of DEFAULT_COLOR/DEFAULT_ICON in PlantFormModal.tsx. They are intentionally decoupled (different semantic domains: manual create vs. packet scan), so this is defensible.
  • The inline validation in onConfirm mirrors SeedLotModal as explicitly documented, which is preferable to premature abstraction of a single-use validation block.
Performance — No material issues found

No material issues found

The PR adds a seed-packet scan UI with no material performance regressions. I verified the key paths:

  • PlantsPage.tsxall and filtered are correctly memoized with useMemo; filterPlants only re-runs when plants.data, query, or category change. No new expensive computations are added to the render path.
  • ScanPacketModal.tsx — The 12 state updates in onSuccess are batched by React 18 in an event handler, resulting in a single re-render. ReadFields is a separate module-level component and only re-renders when proposal or unit change. No work is done inside hot loops; candidate mapping is bounded by the vision model response.
  • seedPacket.ts — Zod parsing of scan/proposal responses is a one-shot cost per network response, not a render-path expense. useCreateFromPacket invalidates both plantsKey and seedLotsKey together, which is the correct minimal invalidation.
  • api.ts — The FormData branch adds a trivial instanceof check per request; no allocations are added to the common JSON path.

The code avoids N+1 queries, unbounded growth, and unnecessary allocations. Nothing in this diff introduces a performance concern worth flagging.

🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

  • ScanPacketModal.tsx:363 — Clicking Rescan clears proposal to return to the capture phase, but it never clears the error state. If the user had an error in review (e.g., "Spacing must be at least 1 in."), that stale error persists and is immediately rendered in the capture phase by {error && <Alert>{error}</Alert>} at line 224.
    Fix: Call setError(null) inside the Rescan click handler, or reset error whenever proposal transitions to null.
opencode/glm-5.2:cloud (opencode) — Minor issues

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

🔒 Security — No material issues found

Verdict: No material issues found.

I reviewed the change through the security lens (authn/authz, injection, SSRF, untrusted-data handling, secret leakage, input validation). Verified against the checked-out code:

  • Untrusted vision output is rendered safely. The SeedPacket fields (species, variety, vendor, sku, lotCode, etc.) come from the vision model and are rendered in ReadFields and the candidate list as React text children, which are auto-escaped — no dangerouslySetInnerHTML anywhere in the diff (verified by grep across *.tsx). No injection vector.
  • Category from the model is validated before use. newPlantDefaults (seedPacket.ts:124) gates suggestedCategory through isPlantCategory against PLANT_CATEGORIES, falling back to 'vegetable'. The UI Select only offers the valid option set. An unknown model-supplied category cannot leak into CreatePlant.
  • Authz is enforced at the service seam, not the UI. POST /seed-lots/scan and /seed-lots/from-packet both go through mustActor(c) (verified in internal/api/seed_packet.go:87,129), and CreateFromPacket enforces the ACL via visiblePlant for an existing-plant selection (internal/service/seed_packet.go:205) and the XOR invariant on plantId/newPlant (:189-192). The frontend adding a new path doesn't bypass the service checks.
  • File upload is server-validated. scanSeedPacket wraps the body in http.MaxBytesReader (30 MiB cap, seed_packet.go:48) and runs imagenorm.Normalize which rejects unsupported/too-large formats before any model call. The client does no pre-validation, but the server is the authority and aborts oversized streams mid-read.
  • No SSRF / secret-leak surface introduced. The vision call uses a server-configured model spec and OLLAMA_CLOUD_API_KEY from the environment (never the DB, per CLAUDE.md); no user-controlled URL is fetched. sourceUrl is hardcoded to '' client-side. postForm keeps credentials: 'same-origin' and lets the browser set the multipart boundary (no attacker-controllable content-type).
  • PlantIcon color/icon injection: the candidates' color/icon originate from the actor's own existing plants (ListPlantsForActor), not from the untrusted vision output, and PlantIcon is already used elsewhere with the same data — no new vector introduced by this PR.

gadfly-findings
[]

🎯 Correctness — Minor issues

Confirmed. The finding is real — ScanPacketModal.tsx:152-153 uses spacingCm < 1 (cm) but the message says 1 ${unitLabel}, which in imperial becomes "1 in" while the actual accepted minimum is ~0.39 in. Faithfully replicated from PlantFormModal.tsx:87-88. Server minPlantSpacingCM = 0.1.

VERDICT: Minor issues

  • web/src/components/plants/ScanPacketModal.tsx:152-153 — The spacing validation rejects spacingCm < 1 (a centimeter threshold) but the error message reads Spacing must be at least 1 ${unitLabel}, which in imperial mode becomes "1 in". Re-deriving: 1 in = 2.54 cm, so the real accepted minimum in inches is ~0.39 in, not 1 in. The message therefore overstates the floor in imperial — a user who types 0.5 in (1.27 cm) is accepted while being told they need "1 in". No valid input is wrongly rejected (the cm threshold is stricter than the stated inch one), so this is cosmetic, but it is misleading. This is a faithful copy of the same wording in PlantFormModal.tsx:87-88, so it's a pre-existing inconsistency propagated into newly written code rather than a fresh regression. Fix would be to state the threshold in the displayed unit, or simply drop the unit from the message ("Spacing must be a positive number"). Verified against internal/service/plants.go:19 (minPlantSpacingCM = 0.1).

Everything else I checked is correct:

  • seedPacketSchema/packetProposalSchema defaults match vision.SeedPacket nullable fields; unknown suggestedCategory falls back to 'vegetable' via isPlantCategory.
  • lotDefaults seed-count → seeds unit, else packets/qty 1 — matches the backend's seedLotFields shape.
  • Candidate pre-selection (candidates[0]?.plant.id ?? 'new') and the XOR of plantId/newPlant match CreateFromPacket's "exactly one" rule.
  • Multipart FormData path in apiFetch correctly omits the JSON content-type so the browser sets the multipart boundary.
  • capabilities.vision default(false) means an older response hides the button rather than failing the parse (verified capabilitiesSchema).
  • The onFile reset-then-read ordering is safe (file captured before e.target.value = '').
🧹 Code cleanliness & maintainability — Minor issues

All four findings verified against the actual source. Let me confirm the grep claims for shared helpers.
All four findings confirmed against the actual source code. No shared helpers exist (grep for validateLot/parseYear/parseCostCents returned nothing), and the duplicated constants/messages are verbatim.

VERDICT: Minor issues

Through the cleanliness/maintainability lens, this is a well-structured change that follows the surrounding patterns (modal shape, mutation hooks, zod schemas in lib/, capability gating). The main cleanliness concern is hand-copied logic and magic constants that the comments themselves claim are "the same as" existing code, but which are decoupled rather than shared — so the comments' promises can silently rot.

  • web/src/components/plants/ScanPacketModal.tsx:105-128 — duplicated lot validation. The onConfirm qty / packed-for-year / cost validation blocks are character-for-character copies of SeedLotModal.tsx:62-88, and the comment at line 105 explicitly says "Lot validation mirrors SeedLotModal so the two paths accept the same things." Verified by reading both files: the Quantity must be a number, or left blank. string, costCents = Math.round(c * 100), and the 1900/2200 year range appear verbatim in both. There is no shared helper in lib/seedLots.ts (confirmed by grep — validateLot/parseYear/parseCostCents don't exist anywhere in web/src). This is exactly the "two paths, one invariant, kept in sync by hope" smell: when SeedLotModal's rules change (and they already differ — germination/sourceUrl/purchasedAt/notes are absent from the scan form), the scan path diverges silently. Suggested fix: extract parseQuantity/parsePackedYear/parseCostCents pure functions into lib/seedLots.ts and call from both modals; the helpers are also unit-testable, which is where the PR already invests test effort.

  • web/src/lib/seedPacket.ts:96-97 — duplicated default color/icon constants. PACKET_PLANT_COLOR = '#4a7c3f' and PACKET_PLANT_ICON = '🌱' are the same values as DEFAULT_COLOR/DEFAULT_ICON in PlantFormModal.tsx:22-23 (verified by grep — both files declare these literals, no shared export). They're renamed and re-declared rather than imported, so the two are coupled by intent ("a new plant with no icon gets this placeholder") but decoupled in code — if the manual form's default changes, the packet path's won't follow and the two create paths will produce visually different defaults for the same conceptual "no icon" state. Suggested fix: export DEFAULT_COLOR/DEFAULT_ICON from a shared spot (e.g. lib/plants.ts next to PlantInput) and import in both, dropping the PACKET_PLANT_* aliases.

  • web/src/lib/seedPacket.ts:98-100DEFAULT_SPACING_CM = 30 claims sameness it can't enforce. The comment says "the same default the manual new-plant form uses", but PlantFormModal.tsx:61 inlines the literal 30 (source?.spacingCm ?? 30) with no reference to this constant (verified by grep — DEFAULT_SPACING_CM appears only in seedPacket.ts, not in PlantFormModal). So the "sameness" is asserted by prose only; if the manual form's literal changes, the comment becomes a silent lie — the exact "stale comments are worse than none" case CLAUDE.md calls out. Suggested fix: share one DEFAULT_SPACING_CM constant (in lib/plants.ts or lib/units.ts) imported by both, or drop the "same default the manual form uses" claim from the comment.

  • web/src/components/plants/ScanPacketModal.tsx:151-163 — duplicated new-plant field validation. The selection === 'new' branch re-implements spacing / days-to-maturity validation that PlantFormModal.tsx:86-100 already has (verified: the spacing < 1 check with the same Spacing must be at least 1 ${unitLabel}. message and "Days to maturity must be a whole number of days" are copied verbatim). Less severe than the lot duplication because the field sets aren't identical (the packet form has no icon/color/sourceUrl/notes, and the name-required message differs), but the spacing + days checks are candidates for a shared validatePlantCoreFields(...) helper if the lot helper above is also extracted. Noting it as the same pattern, not a separate must-fix.

Performance — No material issues found

I've verified the key paths against the checked-out code. Performance lens:

  • useScanPacket/useCreateFromPacket are standard one-shot mutations; no caching concerns (intentionally uncached, each photo is fresh).
  • The scan uploads the raw photo with no client-side downscaling. This is the dominant cost of the flow (multi-MB phone photo → 60s read timeout was added server-side precisely for this), but it's a deliberate architecture choice: HEIC can only be decoded server-side (imagenorm), and normalization is centralized there. Not a regression introduced by this PR, and client-side pre-resize would only help the JPEG/WebP subset. Not flagging.
  • categoryOptions/unitOptions are module-level constants, not rebuilt per render — good.
  • proposal.candidates is bounded server-side (maxPlantMatches = 10 in internal/service/plants.go:91), so the .map in the review phase is bounded. Verified.
  • The 11 setState calls in scan.mutate.onSuccess all fire inside one event-handler callback, so React 18 auto-batches them into a single re-render. Fine.
  • useCreateFromPacket.onSuccess invalidates plantsKey and seedLotsKey — both queries already run on PlantsPage, so the refetch is the expected, necessary cost of showing the new rows. No over- or under-invalidation.
  • ReadFields builds a small rows array per render (≤5 entries); trivial.
  • useCapabilities() on PlantsPage adds one fetch, cached with staleTime: 60_000; not a hot path.

No efficiency regressions, unbounded growth, N+1, or avoidable hot-loop work in this change.

VERDICT: No material issues found

(Performance lens clean; verified the candidate cap, the invalidation targets, and that the heavy image payload is a pre-existing server-side-normalization design decision, not a new regression.)

🧯 Error handling & edge cases — No material issues found

I'll investigate the PR through the error handling & edge cases lens. Let me verify key areas against the actual repo.
Now let me verify the PlantInput/seedLot/LotInput shapes to check the lot object built in ScanPacketModal against the actual SeedLotInput and PlantInput.
Let me check cmFromSpacing and spacingFromCm to verify NaN handling, and the backend seed-packet endpoints for what shapes they actually accept.
parseFloat('') returns NaN, and cmFromSpacing(NaN, ...) returns roundCm(NaN*...). Let me check roundCm:
cmFromSpacing(NaN, unit)roundCm(NaN)NaN. Then !Number.isFinite(spacingCm) || spacingCm < 1 catches NaN. Good.

Now let me verify the backend endpoint to understand what happens with newPlant field ordering / extra fields, and whether the lot object's fields match what's accepted. Let me also check whether purchasedAt: null and other nulls are acceptable server-side.
Now let me verify the API handler's binding shape for from-packet — particularly whether plantId (lowercase d) in the JSON is decoded correctly given the service uses PlantID.
Now let me verify the seedLotFields server-side to check the lot object built in the modal matches (esp. purchasedAt: null vs optional string), and plantCreateRequest to check newPlant field shape (e.g. daysToMaturity null handling, sourceUrl etc.).
Confirmed: server side purchasedAt *string, costCents *int, germinationPct *float64, packedForYear *int, daysToMaturity *int. The modal sends purchasedAt: null, costCents (number|null), germinationPct: null, packedForYear (number|null), daysToMaturity (number|null). All decode fine into Go pointers/nullable.

Now let me look at one edge case in the modal: the cmFromSpacing(parseFloat(spacing), unit) when spacing is empty string. In the selection === 'new' path, validation catches NaN. Good.

Let me also double-check: onFile resets e.target.value = '' then reads e.target.files?.[0] afterward. The reset happens before reading the file — does clearing .value also clear .files?
The e.target.value = '' after grabbing file is a well-known safe pattern — file holds the File reference, reset of .value just clears the input. That's fine.

Now let me consider an important edge case in the error-handling lens: what if scan.mutate is called while a previous scan is pending (rapid double-pick)? React Query mutations aren't queued by default; mutate while pending — default mutationKey none — it'll just start a new mutation. Could cause two proposals landing and the second onSuccess racing. But this is minor and not a crash. Also, the file input is hidden and only triggered when !scan.isPending, so the button itself is gated.

Let me check the ReadFields rendering with a NaN edge case — spacingFromCm(p.spacingCm, unit) where p.spacingCm could be 0 or negative from a bad model read. The schema allows any number. Let me check whether negative spacing could slip through.
Now let me think about edge cases more carefully:

  1. spacingFromCm(p.spacingCm, unit) in ReadFields (line 386 of modal): if p.spacingCm is negative or 0, this renders a weird value. But it's read-only display — not a crash, minor.

  2. newPlantDefaults spacing: p.packet.spacingCm ?? DEFAULT_SPACING_CM — if model returns spacingCm: 0 (printed "0"), prefill becomes 0. Then in review phase, user editing "new" path: cmFromSpacing(parseFloat(spacing), unit) where spacing="0" → spacingCm=0!Number.isFinite(0) || 0 < 10 < 1 true → error "Spacing must be at least 1". That's caught. But the default prefilled value 0 would fail validation if user just clicks submit unchanged. Actually it would show the error — that's handled, not a crash. Acceptable.

  3. Quantity: Number('') is 0, but qty = quantity.trim() === '' ? 0 : Number(quantity). Number('abc') is NaN, caught. Number('1e308') etc. fine. Number('-5') → -5, caught by < 0. Good. But Number('Infinity') → Infinity, Number.isFinite(Infinity) false → error. Good.

  4. Cost: Number(cost) then Math.round(c * 100). c could be huge. Number.MAX_SAFE_INTEGER * 100 overflow not really an issue for typical currency. Fine.

  5. packedForYear: Number(packedForYear)Number('2026.5') = 2026.5, Number.isInteger(2026.5) false → error. Good. Number('1e4') = 10000, integer, in range. Edge: scientific notation accepted but unusual; fine.

Now the main edge case I want to flag: In onConfirm, the existing-plant path (selection !== 'new') sets plantId = selection but does NOT validate that this plant id still exists/is visible. The backend handles that (visiblePlant → ErrNotFound). But more importantly, when selection is a candidate plant id, and newPlant is left undefined — good, exactly-one satisfied. Backend rejects both/neither. Fine.

But there's a subtle issue: if proposal.candidates is empty, setSelection('new') and the radio shows "Add as a new variety". Good. If user selects an existing candidate then the form's name/spacing fields are hidden (only shown when selection === 'new'). On submit with existing selection, newPlant undefined, plantId set. Good.

Now, one real concern: cmFromSpacing(parseFloat(spacing), unit) only happens in the selection === 'new' branch. Good — existing-plant path doesn't need spacing. Fine.

Let me check whether there's an issue with the cost field being non-finite but the Math.round(c * 100) producing NaN that slips to backend. No — !Number.isFinite(c) || c < 0 guards before Math.round. Good.

Let me verify the errorMessage for the 503 VISION_DISABLED case actually surfaces a usable message (the backend writes error.message). The messageFrom reads body.error.message. Good. The onError fallback is only used if not an ApiError. Fine.

Let me check one more: the scan.mutate onError — does React Query call onError for a zod parse failure inside mutationFn? The packetProposalSchema.parse throws a ZodError, which propagates out of mutationFn → React Query treats as mutation error → onError fires with the ZodError. errorMessage(ZodError) → not ApiError → fallback "Couldn't read that photo...". That's a misleading message for a parse failure (server returned unexpected shape), but it degrades gracefully rather than crashing. Minor.

Now let me look at the PlantsPage integration to verify the capabilities.data?.vision gate handles capabilities still loading / error.
The capabilities.data?.vision optional-chain handles data being undefined (loading/error) → falsy → button hidden. Good. The vision default(false) ensures older backends hide it. Good.

Now let me check one potential real issue: when capabilities query errors (network), data is undefined, button hidden — fine, no crash. When the query is stale and shows old vision:true then backend disabled vision — the modal's scan would 503, handled via onError. Fine.

Let me check the Modal busy prop behavior — does busy block the close (X) button? If a scan is pending and user hits Escape or backdrop, does it cancel? Let me check Modal.
Modal busy blocks Escape and backdrop close. Good — so during a scan the modal stays. The Cancel buttons are disabled={busy}. Good. The "Rescan" button (sets proposal(null)) is disabled={busy} — good, can't rescan mid-scan.

But wait — there's a subtle edge: busy = scan.isPending || create.isPending. During review phase, scan.isPending is false (scan done) and create.isPending is false until submit. So busy is false in review — fine. During capture, scan.isPending true → busy true. Good.

One thing: in the review phase, the hidden file input is not rendered (only in capture phase). So user can't re-pick a photo while reviewing — they must click "Rescan" to go back to capture. Fine, that's intentional.

Now let me check whether scan.mutate being called when a previous create is pending could interleave. The capture phase only shows when !proposal, and proposal is set on scan success. Once in review, capture input not shown. So no double-scan. Fine.

Let me check the cost edge: cost.trim() then Number(cost) (not cost.trim()). Minor inconsistency but Number(' 4.99 ') = 4.99, fine. Actually they check cost.trim() for emptiness but pass cost (untrimmed) to Number — Number trims whitespace anyway. Fine.

Now let me look at the most important potential real issue from the error-handling lens: setPackedForYear(lot.packedForYear != null ? String(lot.packedForYear) : '')lot.packedForYear is number | null from lotDefaults. If it's a number, String(number). Fine. But lotDefaults returns packedForYear: p.packedForYear where p.packedForYear is number | null. Good.

Let me verify the spacing default display: setSpacing(String(spacingFromCm(plant.spacingCm, unit))). plant.spacingCm from newPlantDefaults is p.packet.spacingCm ?? DEFAULT_SPACING_CM → always a number. spacingFromCm returns a number. String(number). If spacingCm is e.g. 30 → "30" metric, or imperial 30cm → 11.8 → "11.8". Fine. If spacingCm were NaN somehow, String(NaN) = "NaN", and on submit parseFloat("NaN") = NaN → caught. Acceptable.

Now let me check the test file briefly to confirm test coverage claims and that tests don't assert wrong things.

The tests look reasonable. Let me focus on one more potential real issue: the lot object sent on confirm is missing nothing required, but unit validation. Backend seedLotFields.Unit has binding:"required". The modal always sends lotUnit (default 'packets'), so always present. Good.

Let me check whether there's an issue with germinationPct: null — backend GerminationPct *float64 accepts null. The SeedLotInput TS type requires germinationPct: number | null. Modal sends null. Good.

Let me verify the SeedLotFields type matches what modal builds (TypeScript-level). SeedLotFields = Omit<SeedLotInput, 'plantId'>. SeedLotInput has: vendor, sourceUrl, sku, lotCode, purchasedAt, packedForYear, quantity, unit, costCents, germinationPct, notes. Modal builds exactly these. Good.

Now, one genuine concern from the error-handling lens: Number(quantity) when quantity is something like " " (whitespace only)quantity.trim() === '' ? 0 : Number(quantity). " ".trim() = "" → 0. Good. But Number(" 5 ") = 5. Good. Number("5abc") = NaN → caught. Good.

Let me check: Number(days) for daysToMaturitydays.trim() then Number(days). Number("90") = 90. Number("90.5") = 90.5, Number.isInteger(90.5) false → error. Good. Number("") would be 0 but guarded by days.trim(). Good.

I think the error handling is quite solid. Let me do a final check on one thing: the onConfirm is async and uses await create.mutateAsync(...). If the component unmounts mid-await (user closes modal? No — busy blocks close). Actually onClose() is called inside the try after success. During pending, busy true blocks backdrop/escape. The Cancel/Rescan buttons disabled. So no unmount-during-await. Good. No setState-after-unmount risk in the success path. The catch sets error — component still mounted. Fine.

Let me check one last potential: create.mutateAsync throwing an ApiError with isConflict (409) — possible if the candidate plant was version-conflicted? Actually creating a lot against an existing plant doesn't carry version. Creating a new plant doesn't either. So 409 unlikely. But if it happens, errorMessage surfaces the message. No special conflict-rebase handling like other forms. That's an inconsistency vs SeedLotModal which calls conflictSeedLot. But for a create there's no version to rebase. Minor, not a crash.

Let me verify SeedLotModal doesn't do conflict rebase on create either (to confirm this isn't a regression).
That conflict rebase is only relevant for edit (PATCH with version). The packet confirm is a create (no version), so 409 won't occur on the lot create itself. Confirmed not a regression.

I've thoroughly checked the error-handling and edge cases. The code is notably defensive: NaN guards, empty-string handling, null/optional server fields, the vision default(false) for older backends, busy-lock preventing close/interleave, and the exactly-one validation delegated to the backend. The unhappy paths (503 VISION_DISABLED, unreadable image, too-large) are all funneled through errorMessage with sensible fallbacks.

The only minor items I found:

  1. A Zod parse failure on the scan response (server returned an unexpected shape) surfaces the generic "Couldn't read that photo…" fallback rather than something indicating a server/protocol mismatch — misleading but not a crash, and an unlikely path given the schema is shared.
  2. spacingFromCm(p.spacingCm, unit) in ReadFields will render a nonsensical value if the model returns a negative/zero spacingCm, but it's a read-only display and the model is instructed to leave absent fields null (so 0/negative is unlikely). Not a crash.

Neither rises to "material" for the error-handling lens. The validation, null-handling, and unhappy-path coverage are solid.

Verdict: No material issues found

Through the error-handling & edge-cases lens, the change is solid. Verified by reading the files:

  • NaN / empty / negative guards in onConfirm (ScanPacketModal.tsx:106-119, :127-135, :138-144): quantity, packedForYear, cost, spacing, days all trim-check emptiness, then Number.isFinite/Number.isInteger/range checks before use. cmFromSpacing(parseFloat(spacing), unit) producing NaN is caught by !Number.isFinite(spacingCm) || spacingCm < 1 (:128). Confirmed roundCm/cmFromSpacing propagate NaN (units.ts:29-31, 191-193), so the guard is load-bearing and correct.
  • Null/optional server fields: the lot object (:121-134) and newPlant (:146-154) match seedLotFields (seed_lots.go:24-36) and plantCreateRequest (plants.go:16-26) — nullable Go pointers accept the nulls the client sends (purchasedAt, costCents, germinationPct, packedForYear, daysToMaturity). Verified the TS SeedLotFields = Omit<SeedLotInput,'plantId'> and the modal builds exactly those keys.
  • vision default(false) (agent.ts) + capabilities.data?.vision gate (PlantsPage.tsx:81): handles loading/error/older-backend gracefully — optional chain yields falsy, button hidden, no dead button, no parse failure. Verified against the schema.
  • Busy-lock prevents interleaving/close-during-pending: busy = scan.isPending || create.isPending flows to Modal busy which blocks Escape/backdrop (Modal.tsx:47,94), and the Cancel/Rescan buttons are disabled={busy} (:319,:323). No setState-after-unmount in the async onConfirm success path (onClose only after await).
  • Scan error funneling: 503 VISION_DISABLED / unreadable / too-large all return error.message via messageFrom (api.ts:80-88), surfaced through errorMessage(err, fallback) (:95-96). A non-ApiError (e.g. a zod parse failure inside mutationFn) falls back to the generic message — degrades gracefully, no crash.
  • Exactly-one validation (plantId XOR newPlant) is enforced server-side (seed_packet.go:189-192); the client always sends exactly one (selection==='new' → newPlant, else → plantId), so neither "both" nor "neither" can occur.
  • e.target.value = '' after grabbing file (:70-73): the File reference is retained; resetting the input only allows re-picking the same file. Correct pattern.

Two non-material observations (not blocking, no crash/incorrect behavior):

  • A Zod parse failure on the scan response surfaces the "couldn't read that photo" fallback rather than a protocol-mismatch indication — unlikely given the shared schema, and it degrades safely.
  • ReadFields renders spacingFromCm(p.spacingCm, unit) for any non-null spacingCm; a pathological 0/negative model read would show a nonsensical read-only value, but the prompt instructs the model to leave unprinted fields null, and confirm-path validation re-checks the editable spacing.
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

Through the Security lens I traced every tainted-data path in the diff:

  • File uploadFormDataapi.postForm → backend http.MaxBytesReaderimagenorm.Normalize (JPEG re-encode). No filename-based path injection; content is normalized server-side before reaching the vision model.
  • Frontend form inputs (name, vendor, sku, lotCode, quantity, cost, spacing, days) are validated (trim, numeric checks, range bounds) before submission. parseFloat/Number on empty strings produce NaN, which the existing !Number.isFinite(...) guard rejects.
  • Candidate plant.id selection is an integer validated by plantSchema on arrival; no path-traversal or injection vector.
  • Capabilities schema now defaults both agent and vision to false — a secure-by-default improvement that hides features on partial/older responses instead of exposing them or crashing the parse.
  • React rendering of all user-supplied strings (proposal fields, form inputs, error messages) goes through React’s normal JSX escaping; no dangerouslySetInnerHTML is introduced.
  • Error messages from the backend (ApiError) are user-facing static strings in the Go handlers; no stack-trace or internal-path leakage.
  • Secrets remain untouched; OLLAMA_CLOUD_API_KEY stays env-only per DESIGN.md, and the UI only sees a boolean vision capability.

Nothing in this PR introduces an auth/authz gap, injection surface, SSRF, deserialization risk, secret leak, or unsafe default.

🎯 Correctness — No material issues found

No material issues found

I reviewed the seed-packet scan UI change through the correctness lens, tracing every validation rule, type boundary, and backend contract. Here's what I checked:

Validation logic in ScanPacketModal.tsx — The onConfirm handler mirrors SeedLotModal exactly: quantity ≥ 0, year 1900–2200, cost ≥ 0, spacing ≥ 1 cm, days ≥ 1 (or null). All edge cases (empty string, NaN, negative, zero) are handled correctly. parseFloat(spacing)cmFromSpacing!Number.isFinite correctly rejects non-numeric and out-of-range inputs.

Unit conversionsspacingFromCm / cmFromSpacing round-trip correctly for both metric and imperial. The roundCm dust-guard (1e-6) is applied consistently.

Backend contract alignmentFromPacketBody sends exactly one of plantId (number) or newPlant (PlantInput), matching the Go struct fromPacketRequest which demands exactly one via PlantID *int64 XOR NewPlant *plantCreateRequest. The lot shape (SeedLotFields = Omit<SeedLotInput, 'plantId'>) matches seedLotFields.toInput() on the server.

Schema defaultsseedPacketSchema defaults every field safely. newPlantDefaults falls back to 'vegetable' for unknown categories (defensive against model hallucination) and 30 cm for missing spacing, matching the manual new-plant form. lotDefaults correctly interprets a positive seedCount as seeds and defaults to 1 packet otherwise.

FormData uploadapi.postForm passes FormData through unchanged with no JSON content-type header, letting the browser set the multipart boundary. The instanceof FormData guard in apiFetch is safe and backward-compatible.

Capabilities schema — Adding vision: z.boolean().default(false) alongside agent means older/partial responses hide the feature rather than failing parse. This is the correct defensive choice.

Query invalidationuseCreateFromPacket invalidates plantsKey and seedLotsKey on success, so the catalog refreshes after confirm.

Everything checks out. The prefill logic, validation, and backend contracts are all consistent.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • web/src/components/plants/ScanPacketModal.tsx:106 — The quantity, packed-for-year, and cost validation blocks are copy-pasted from SeedLotModal.tsx:62-88 (same messages, same logic). The PR description even calls this out ("mirrors SeedLotModal"). Extract shared validators (e.g. validateLotQuantity, validateYear, validateCost) so a rule change doesn't need two updates.

  • web/src/components/plants/ScanPacketModal.tsx:26categoryOptions is identical to the one in PlantFormModal.tsx:25. It should be exported once from @/lib/plants (e.g. export const categoryOptions = …) and imported by both modals.

  • web/src/components/plants/ScanPacketModal.tsx:27unitOptions duplicates the inline expression in SeedLotModal.tsx:167. It should be exported once from @/lib/seedLots (e.g. export const lotUnitOptions = …) and shared.

Performance — No material issues found

No material issues found.

The frontend-only change is a well-contained modal dialog with local form state. From a performance lens:

  • Image upload: No client-side size limiting or compression before the multipart POST, so a modern phone camera's full-resolution JPEG (~2–5 MB) travels uncompressed. This is worth keeping in mind for mobile bandwidth and server memory, but it's a UX/API design choice inherited from the already-live backend (#94), not a new frontend regression introduced by this diff.
  • Query invalidation: useCreateFromPacket broadly invalidates plantsKey and seedLotsKey. These are simple unparameterized keys and match the existing invalidation patterns elsewhere in the codebase (e.g. plants.ts, seedLots.ts), so there's no new cache thrashing.
  • Rendering: The candidate list renders directly from the backend response. matchPlants on the backend doesn't cap results, but its matching logic (exact name → variety-in-name → same species word) is conservative enough that runaway lists are unlikely in practice, and this is pre-existing backend behavior.

Nothing in the changed code introduces N+1 queries, quadratic loops, unnecessary allocations inside hot paths, or blocking work on render.

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

I scrutinized the diff specifically through the error-handling and edge-cases lens. Here is what I checked and why it is clean:

  • Nil / empty inputs: Every form field that can be left blank is guarded (quantity.trim() === '' ? 0, cost.trim(), days.trim(), packedForYear.trim()). The file-input reset (e.target.value = '') is done before the early return so re-selecting the same file after an error always fires onChange.
  • Numeric validation: Number.isFinite, Number.isInteger, and range checks (< 1, < 0, < 1900, > 2200) are applied to every parsed field. parseFloat('')NaN is caught by !Number.isFinite. Number('abc')NaN is likewise caught.
  • Spacing edge cases: cmFromSpacing(parseFloat(spacing), unit) is validated with !Number.isFinite(spacingCm) || spacingCm < 1, which catches NaN, Infinity, negative values, and zero/negative-zero.
  • Overflow: costCents = Math.round(c * 100) could theoretically exceed Number.MAX_SAFE_INTEGER for absurd inputs, but this is a server-validated field and the client guards (Number.isFinite, c < 0) are appropriate for the UI layer.
  • Abort / unmount during async work: busy = scan.isPending || create.isPending is piped into the Modal’s busy prop, which disables the backdrop-click and Escape close paths as well as the Cancel/Rescan buttons. The only remaining unmount path is a full page navigation; in React 18 state updates on an unmounted component are silently discarded, so this does not produce a crash or leak.
  • Schema fallback safety: capabilitiesSchema defaults both agent and vision to false, so an older/partial server response hides the feature rather than failing parse. seedPacketSchema defaults every field, and newPlantDefaults falls back unknown categories to 'vegetable' and missing spacing to 30 cm.
  • FormData multipart path: The instanceof FormData check in apiFetch correctly omits the JSON content-type so the browser generates the multipart boundary. postForm is a thin, typed wrapper and is used only in the browser.

Nothing in this diff ignores errors, swallows exceptions, leaks resources, or mishandles boundary conditions in a way that reaches the user or breaks the UI.

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: Blocking issues found** · 10 findings (3 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟠 | Lot validation (qty/year/cost) hand-copied from SeedLotModal with no shared helper; comment claims mirroring but nothing enforces it | `web/src/components/plants/ScanPacketModal.tsx:105` | 4/5 | maintainability | | 🔴 | Stale review-phase error persists after clicking Rescan | `web/src/components/plants/ScanPacketModal.tsx:363` | 2/5 | error-handling | | 🟡 | New-plant spacing/days validation copied verbatim from PlantFormModal; same duplication pattern as the lot validation | `web/src/components/plants/ScanPacketModal.tsx:151` | 2/5 | correctness, maintainability | <details><summary>7 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🔴 | Cost field persists across rescans because setCost('') is missing in onFile success handler | `web/src/components/plants/ScanPacketModal.tsx:88` | kimi-k2.6:cloud | correctness | | 🟠 | parseFloat(spacing) is less strict than Number() used for other fields, allowing malformed spacing input | `web/src/components/plants/ScanPacketModal.tsx:143` | kimi-k2.6:cloud | correctness | | 🟡 | categoryOptions duplicated from PlantFormModal | `web/src/components/plants/ScanPacketModal.tsx:26` | opencode/kimi-k2.6:cloud | maintainability | | 🟡 | Hand-built 'lot' literal re-spells SeedLotFields instead of spreading lotDefaults + overrides | `web/src/components/plants/ScanPacketModal.tsx:130` | glm-5.2:cloud | maintainability | | 🟡 | No cancel/timeout for the scan mutation; Modal blocks all close paths while busy, trapping the user during a slow/stuck vision call | `web/src/components/plants/ScanPacketModal.tsx:191` | claude-code/sonnet | error-handling | | 🟡 | PACKET_PLANT_COLOR/PACKET_PLANT_ICON duplicate PlantFormModal's DEFAULT_COLOR/DEFAULT_ICON under new names; same value coupled by intent, decoupled in code | `web/src/lib/seedPacket.ts:96` | opencode/glm-5.2:cloud | maintainability | | ⚪ | 14 useState slots in one ~330-line component; a reducer/single review-state object would reduce the setter wall | `web/src/components/plants/ScanPacketModal.tsx:47` | glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> VERDICT: No material issues found I reviewed this diff specifically through a security lens (authn/authz, injection, unsafe deserialization, secret leakage, untrusted-input handling), verifying against the checked-out repo rather than the diff text alone: - **`web/src/lib/api.ts`** (`apiFetch`/`postForm`) — confirmed the `FormData` branch never sets a manual `content-type`, so the browser generates the multipart boundary correctly; the JSON branch is unaffected. `credentials: 'same-origin'` is preserved, so the session cookie handling didn't change. No injection surface here — `path` values passed to `api.postForm`/`api.post` are static string literals (`/seed-lots/scan`, `/seed-lots/from-packet`), not built from user input. - **`ScanPacketModal.tsx`** — all model-extracted / candidate data (`p.species`, `c.reason`, `c.plant.name`, etc.) is rendered as plain JSX text content, no `dangerouslySetInnerHTML` or similar sink. `selection`/`plantId` is constrained to `number | 'new'` and the numeric branch only ever comes from `c.plant.id` in the server-returned candidate list (via the radio inputs) — not free-typed, so it can't be used to reference an arbitrary/unauthorized plant ID through client tampering beyond what the server itself already returned for this user. - **`seedPacket.ts`** — all response data passes through zod `.parse()` with `.default()` fallbacks before use; an unexpected/malicious category string is defensively re-validated client-side (`isPlantCategory`) in addition to the server-side check the comment notes, so a bad `suggestedCategory` can't leak an invalid enum value into the create payload. - **`internal/api/seed_packet.go`** (unchanged, confirmed via grep) — the existing backend already enforces auth (401 on no session), payload size limits (413), and vision-disabled gating (503), and this PR is frontend-only, so no backend authz/validation surface was touched or bypassed. The client-side `capabilities.vision` check is purely a UX gate (hide vs. show a button); it grants no access itself since the server independently gates the vision call. - Capability schema change (`agent`/`vision` now `.default(false)`) fails safe: a partial/malformed `/capabilities` response hides features rather than exposing them, which is the correct direction. No taint path from the untrusted image/model output to a dangerous sink, no new unauthenticated endpoint, no secret handling in this diff. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> Modal already guards busy-close, so no issue there. Confirmed the diff is well-aligned with the backend contract and existing form patterns. I've verified schema field names, units math, and JSON shapes against the Go backend and sibling components — nothing correctness-wise stands out. **VERDICT: No material issues found** Verification performed: - Cross-checked `seedPacketSchema`/`packetProposalSchema`/`packetResultSchema` (web/src/lib/seedPacket.ts) field-by-field against `vision.SeedPacket`, `service.PacketProposal`/`PacketConfirm`/`PacketResult`, and the `fromPacketRequest`/`plantCreateRequest`/`seedLotFields` JSON tags in `internal/api/{seed_packet,plants,seed_lots}.go` — names and optionality match. - Re-derived the unit math: `cmFromSpacing`/`spacingFromCm` (web/src/lib/units.ts:141-152) and `DEFAULT_SPACING_CM = 30` match the existing `PlantFormModal` conventions exactly (same default, same `spacingCm < 1` floor, same round-trip via `roundCm`). - Verified `Selection = number | 'new'` against `plantSchema.id: z.number()` (web/src/lib/plants.ts) — no type mismatch, and the `?? 'new'` (not `||`) correctly treats a candidate id of `0` as valid. - Verified `plantId`/`newPlant` are mutually exclusive when serialized (JSON.stringify drops `undefined`), matching the backend's `hasID == hasNew` XOR check in `CreateFromPacket` (internal/service/seed_packet.go:189-192). - Confirmed `isPlantCategory`/`PLANT_CATEGORIES` matches the backend's `plantCategories` set used by `validCategory` (vegetable/herb/flower/fruit/tree_shrub/cover) — an unknown/model-hallucinated category can't slip through. - Confirmed `apiFetch`'s new FormData branch omits the JSON content-type header (letting the browser set the multipart boundary) and that the field name `image` matches `c.FormFile("image")` server-side. - Confirmed `Modal`'s `busy` flag blocks Escape/backdrop close during the scan/create mutations, so no race with unmounting mid-request. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Confirmed line-by-line: lines 106-127 in `ScanPacketModal.tsx` match `SeedLotModal.tsx:62-88` exactly (same conditions, same error strings, same 1900/2200 bounds), and lines 151-164 match `PlantFormModal.tsx:86-100` exactly (same spacing/days parsing and error strings). The draft's finding is accurate. ## Verdict: Minor issues - **`web/src/components/plants/ScanPacketModal.tsx:106-127`** duplicates the quantity/packedForYear/cost validation verbatim from `web/src/components/plants/SeedLotModal.tsx:62-88` (identical conditions, error strings, and the `1900`/`2200` year bounds — confirmed by direct comparison of both files), and **`ScanPacketModal.tsx:151-164`** separately duplicates the spacing/days-to-maturity validation from `web/src/components/plants/PlantFormModal.tsx:86-100`. This one new file now carries a second copy of both other modals' parsing logic, so there are three independent places encoding "how to parse a packed-for year" / "cost" / "days to maturity" — a future change to those bounds or messages must be applied in three places to stay consistent. Extracting a shared helper (e.g. `parseLotFields(...)` returning `{value} | {error}`) would remove the duplication and the drift risk. - The codebase already tolerates some inline duplication between `SeedLotModal` and `PlantFormModal`, so this isn't a new convention violation, but this PR compounds it with a third and fourth copy in one file. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** Reviewed through the Performance lens: this is a small, self-contained frontend feature (a scan modal, a schema/prefill helper module, and a `postForm` addition to the API client). Checked for the usual suspects: - No loops over unbounded data — `proposal.candidates` and the various form arrays (`PLANT_CATEGORIES`, `LOT_UNITS`) are small, fixed-size, module-scope constants (verified in `web/src/lib/plants.ts:10` and `web/src/lib/seedLots.ts:13-20`), not recomputed per render. - The scan (`useScanPacket`) is correctly a one-shot, uncached mutation — appropriate since each photo is unique; no risk of stale-cache growth. - `useCreateFromPacket`'s `onSuccess` invalidates exactly `plantsKey` and `seedLotsKey` (`web/src/lib/seedPacket.ts:66-69`) — matches the two catalogs actually affected, no over-invalidation cascading unrelated queries. - The multipart path in `api.ts` (`web/src/lib/api.ts:98-101`) passes the `FormData` through as-is with no added serialization/copy step — no unnecessary buffering of the image. - `newPlantDefaults(proposal)` is called twice across the component lifecycle (once for prefill in `onFile`, once again in `onConfirm`), but it's a cheap plain-object construction on a small, already-parsed proposal — not a measurable cost. - No new polling, no added blocking calls on any hot path; the only slow call (vision scan) is already gated behind an explicit user action and a pending/spinner state, not something that runs repeatedly or in a loop. Nothing here rises to a real efficiency regression. This is frontend-only, one-shot-interaction code with no data-scale exposure. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **VERDICT: Minor issues** - `web/src/components/plants/ScanPacketModal.tsx:191` (with `Modal`'s blocking logic at `web/src/components/ui/Modal.tsx:47,94`): while a scan is in flight, there is no way to cancel or bail out. `Modal` disables Escape (`Modal.tsx:47`, gated on `!busyRef.current`) and backdrop-click-to-close (`Modal.tsx:94`, gated on `!busy`) whenever `busy` is true, and `ScanPacketModal`'s own capture-phase Cancel button is `disabled={busy}` (`ScanPacketModal.tsx:227`). Confirmed `busy = scan.isPending || create.isPending` (`ScanPacketModal.tsx:67`). Confirmed server-side the scan endpoint deliberately extends deadlines well past instant-CRUD expectations (`internal/api/seed_packet.go:30,37`: `scanReadTimeout = 60s`, `scanWriteTimeout = 120s`), because a slow phone upload plus a live vision call can legitimately run long — the modal's own copy only promises "this can take a few seconds" (`ScanPacketModal.tsx:216`). Confirmed neither `useScanPacket`'s `mutationFn` (`web/src/lib/seedPacket.ts:70-78`, `async (file: File)`, no signal threaded through) nor `apiFetch` is ever invoked with an `AbortSignal` for this call, even though `RequestOptions.signal` exists and is honored (`web/src/lib/api.ts:50,106`) — it's simply never passed here. Net effect: a hung or slow vision call leaves the user with no in-app way to close the dialog short of reloading the page, which is a materially worse failure mode than the same "block close while busy" convention applied to sub-second CRUD modals like `SeedLotModal`/`PlantFormModal`. Suggested fix: wire an `AbortController` into the scan mutation and let Cancel/Escape abort it, or allow closing after a shorter client-side timeout with a clear message. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **Verdict: No material issues found** Through the security lens I traced the tainted-data path in this PR: server-vision output is the untrusted input, and it flows through `seedPacketSchema`/`packetProposalSchema` (zod parse) before being rendered or sent back. Verified the key points: - **No injection / no `dangerouslySetInnerHTML`**: grep confirms none in `web/src`. The extracted packet fields (`species`, `variety`, `reason`, etc.) are rendered as React text children (`{c.reason}`, `<dd>{v}</dd>`, `TextField value=...`), so they're auto-escaped — no XSS sink. Verified `Alert.tsx` renders children as plain ReactNode, not raw HTML. - **Untrusted model output is schema-validated before use**: `useScanPacket` parses via `packetProposalSchema` (and nested `plantSchema`/`seedPacketSchema`) before the data reaches state; `useCreateFromPacket` parses the result via `packetResultSchema`. Unknown category is explicitly neutralized to `'vegetable'` via `isPlantCategory` rather than passed through — a malformed/attacker-influenced model response can't smuggle an arbitrary category into `PlantInput`. - **Multipart upload is sent with no manual content-type**: `api.postForm` lets the browser set the multipart boundary; the diff correctly omits the JSON content-type for `FormData` (`api.ts:99-112`). No header injection / boundary-clobber path. - **Confirm body is built from user-edited, validated fields**: client-side validation (`qty`, `year` range, `costCents`, `spacingCm`) mirrors the existing lot form; the server is the real authority per the backend design, so these are UX, not a security boundary — but no unvalidated server string is reflected back unsanitized. - **CSRF/auth**: upload uses `credentials: 'same-origin'` (session cookie) same as every other call; no new auth surface introduced. Gating the entry point on `capabilities.vision` prevents a dead/404-ing button but isn't a security control, and the 503 path is handled. Nothing in my lane materially wrong. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> **Verdict: Minor issues found** - **`web/src/components/plants/ScanPacketModal.tsx:153` — spacing validation floor is 1 cm but the error message quotes the display unit.** The guard is `spacingCm < 1` where `spacingCm = cmFromSpacing(parseFloat(spacing), unit)` (`cmFromSpacing` returns `value * CM_PER_INCH` for imperial, `CM_PER_INCH = 2.54`, at `web/src/lib/units.ts:191-203,7`), so the real floor is 1 cm. The message reads `` `Spacing must be at least 1 ${unitLabel}.` `` and `spacingUnitLabel` returns `in` for imperial. In metric this is correct ("1 cm"); in imperial the message says "at least 1 in" while the actual floor is ~0.39 in (1 cm). A user entering `0.5` in (≈1.27 cm) passes despite being told the minimum is 1 in, and `0.4` in (≈1.0 cm) is rejected with a message claiming the floor is 1 in. Fix: state the threshold in cm universally, or compute a unit-specific floor (`cmFromSpacing(1, unit)` / `spacingFromCm(1, unit)`) and compare/display in the entry unit. (Verified against `units.ts:191-203` and the `<TextField min="1">` at line ~245, which has the same cm/in mismatch in imperial.) </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All findings verified against the actual source. Findings 1–3 confirmed; finding 4 contradicted by the code (the three constants are already consecutive — `isPlantCategory` follows them, it doesn't split them). ## Review: 🧹 Code cleanliness & maintainability **VERDICT: Minor issues** - **`ScanPacketModal.tsx:105–128` — duplicated lot-validation block.** The quantity/year/cost parsing and the three error strings (`'Quantity must be a number, or left blank.'`, `'Packed-for year should be a four-digit year.'`, `'Cost must be an amount, or left blank.'`) plus `Math.round(c * 100)` are copy-pasted from `SeedLotModal.tsx:62–88`. The comment on line 105 even admits it ("Lot validation mirrors SeedLotModal"). A fix to the year range or cost rounding in one modal won't reach the other. Suggested fix: extract a pure `parseLotFields(...)` helper returning `{ values, error }` into `lib/seedLots.ts` and call it from both modals. Low churn, one source of truth. - **`ScanPacketModal.tsx:130–142` — hand-built `lot` object re-spells `SeedLotFields`.** The literal sets every field of `SeedLotFields` (`seedPacket.ts:57`), hard-coding `sourceUrl: ''`, `purchasedAt: null`, `germinationPct: null`, `notes: ''` to the exact values `lotDefaults` already returns (`seedPacket.ts:131–147`); the only deltas are the user-edited `quantity`/`unit`/`costCents`/`packedForYear`/`sku`/`lotCode`/`vendor`. `lotDefaults` is already imported and used in the prefill `onSuccess`, so `{ ...lotDefaults(proposal.packet), <edited overrides> }` would remove the duplication and keep the two paths from diverging on which fields exist. - **`ScanPacketModal.tsx:47–64` — 14 `useState` slots in one ~330-line component.** The component mixes capture, prefill, validation, and confirm in one body, and the prefill `onSuccess` (lines ~78–93) scatters `newPlantDefaults`/`lotDefaults` results across a wall of setters. Readable now, but a `useReducer` or single `reviewState` object for the review-phase fields would flatten the setter wall and make prefill a single assignment — a natural follow-up rather than something to force in this PR. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found Reviewed through the performance lens: the new modal holds a single `File` in React state (transient, GC'd after the mutation), `ReadFields` builds a small array of rows from a handful of proposal fields (bounded, not in a hot path), the candidates list is rendered once (model-gated, tiny N), and the confirm invalidates exactly the two catalog query keys (`plantsKey`, `seedLotsKey`) — no fan-out or N+1. `postForm` correctly avoids `JSON.stringify` and lets the browser stream the multipart body. `useScanPacket` is uncached by design (one-shot per photo), so no unbounded growth. Nothing here is a material efficiency regression. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Confirmed. The finding is valid: Rescan handler at line 363 is `() => setProposal(null)` with no `setError(null)`; the shared `error` state is rendered in both the capture phase (line 224) and review phase (line 360); a confirm failure sets it to "Could not save the packet." (line 186); and it only clears once a new file is picked (`onFile` at line 75). The draft cited line 284, but the actual Rescan button is at line 363 — correcting that. **Verdict: Minor issues** - **Stale error persists into the capture phase after "Rescan."** `ScanPacketModal.tsx:363` — the Rescan button's handler is `() => setProposal(null)` with no `setError(null)`. If a confirm attempt failed (e.g. server rejected the lot), `error` holds "Could not save the packet." (set at `ScanPacketModal.tsx:186`). Clicking Rescan returns the user to the *capture* phase, which renders the same shared `{error && <Alert>}` block (`ScanPacketModal.tsx:224`), so the misleading save-failure message is shown over the "Take or choose a photo" UI. It only clears once the user actually picks a new file (because `onFile` calls `setError(null)` at `ScanPacketModal.tsx:75`). `error` is one shared state variable rendered in both phases. Suggested fix: clear `error` in the Rescan handler, e.g. `onClick={() => { setProposal(null); setError(null) }}`. No other material error-handling issues: the scan `onError` correctly surfaces the server message, the confirm try/catch wraps only the awaited mutation (validation failures `return` rather than throw), `void qc.invalidateQueries` is the intended fire-and-forget, and the null/empty guards in `ReadFields` (`p.spacingCm != null`, `p.seedCount != null`) and `lotDefaults` (`seedCount != null && seedCount > 0`) are correct. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found I reviewed the seed-packet scan UI changes through the security lens. The file upload path, server response handling, form rendering, and toast notifications are all safe. Key observations: - **File upload**: `api.postForm` correctly omits the `content-type` header for `FormData`, letting the browser generate the multipart boundary. No path traversal or injection risk — the `File` object is passed as-is. - **XSS prevention**: Extracted packet data (`species`, `variety`, etc.) is rendered as React text nodes in `ReadFields` and form inputs — no `dangerouslySetInnerHTML`, no HTML injection surface. Toast and Alert components also render text, not HTML. - **Response validation**: Server responses from `/seed-lots/scan` and `/seed-lots/from-packet` are parsed through Zod schemas (`packetProposalSchema`, `packetResultSchema`), preventing malformed data from propagating. - **No secret leakage**: No hardcoded credentials, tokens, or API keys introduced. - **Capability gating**: The scan button is correctly hidden when `capabilities.vision` is false, preventing 404/503 exposure. - **Client-side validation**: Quantity, year, cost, spacing, and days are validated before submission, complementing server-side checks. The changes are clean from a security perspective. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Blocking issues found** * **`web/src/components/plants/ScanPacketModal.tsx:88-95`** — The `cost` form state is never reset when a new packet is scanned. `onFile` sets `name`, `category`, `spacing`, `days`, `vendor`, `quantity`, `lotUnit`, `sku`, `lotCode`, and `packedForYear` from the new proposal, but `setCost('')` is missing. Consequently, if a user enters a cost for one packet, clicks "Rescan", and scans a different packet, the old cost value silently persists and will be submitted with the new lot. **Fix:** add `setCost('')` in the `onFile` success handler. * **`web/src/components/plants/ScanPacketModal.tsx:143`** — Spacing validation uses `parseFloat(spacing)`, while `quantity` and `days` use the stricter `Number()` (lines 106, 149). `parseFloat` accepts trailing garbage (e.g. `"15abc"` → `15`), so malformed spacing input can slip through where other fields would be rejected. Since the input is `type="number"` this is harder to trigger in practice, but the inconsistency weakens validation. **Fix:** replace `parseFloat(spacing)` with `Number(spacing)` for consistency. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **No material issues found** The code is well-structured and follows existing patterns. A few observations, none material: - `ScanPacketModal` is large (~400 lines) but is appropriately a single cohesive unit — a two-phase modal where the review form's fields are tightly coupled to the capture phase. Further splitting would add indirection without improving readability. - `PACKET_PLANT_COLOR`/`PACKET_PLANT_ICON` in `seedPacket.ts` duplicate the values of `DEFAULT_COLOR`/`DEFAULT_ICON` in `PlantFormModal.tsx`. They are intentionally decoupled (different semantic domains: manual create vs. packet scan), so this is defensible. - The inline validation in `onConfirm` mirrors `SeedLotModal` as explicitly documented, which is preferable to premature abstraction of a single-use validation block. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** The PR adds a seed-packet scan UI with no material performance regressions. I verified the key paths: - **`PlantsPage.tsx`** — `all` and `filtered` are correctly memoized with `useMemo`; `filterPlants` only re-runs when `plants.data`, `query`, or `category` change. No new expensive computations are added to the render path. - **`ScanPacketModal.tsx`** — The 12 state updates in `onSuccess` are batched by React 18 in an event handler, resulting in a single re-render. `ReadFields` is a separate module-level component and only re-renders when `proposal` or `unit` change. No work is done inside hot loops; candidate mapping is bounded by the vision model response. - **`seedPacket.ts`** — Zod parsing of scan/proposal responses is a one-shot cost per network response, not a render-path expense. `useCreateFromPacket` invalidates both `plantsKey` and `seedLotsKey` together, which is the correct minimal invalidation. - **`api.ts`** — The `FormData` branch adds a trivial `instanceof` check per request; no allocations are added to the common JSON path. The code avoids N+1 queries, unbounded growth, and unnecessary allocations. Nothing in this diff introduces a performance concern worth flagging. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **Blocking issues found** - **`ScanPacketModal.tsx:363`** — Clicking **Rescan** clears `proposal` to return to the capture phase, but it never clears the `error` state. If the user had an error in review (e.g., "Spacing must be at least 1 in."), that stale error persists and is immediately rendered in the capture phase by `{error && <Alert>{error}</Alert>}` at line 224. **Fix:** Call `setError(null)` inside the Rescan click handler, or reset `error` whenever `proposal` transitions to `null`. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verdict: No material issues found. I reviewed the change through the security lens (authn/authz, injection, SSRF, untrusted-data handling, secret leakage, input validation). Verified against the checked-out code: - **Untrusted vision output is rendered safely.** The `SeedPacket` fields (species, variety, vendor, sku, lotCode, etc.) come from the vision model and are rendered in `ReadFields` and the candidate list as React text children, which are auto-escaped — no `dangerouslySetInnerHTML` anywhere in the diff (verified by grep across `*.tsx`). No injection vector. - **Category from the model is validated before use.** `newPlantDefaults` (`seedPacket.ts:124`) gates `suggestedCategory` through `isPlantCategory` against `PLANT_CATEGORIES`, falling back to `'vegetable'`. The UI `Select` only offers the valid option set. An unknown model-supplied category cannot leak into `CreatePlant`. - **Authz is enforced at the service seam, not the UI.** `POST /seed-lots/scan` and `/seed-lots/from-packet` both go through `mustActor(c)` (verified in `internal/api/seed_packet.go:87,129`), and `CreateFromPacket` enforces the ACL via `visiblePlant` for an existing-plant selection (`internal/service/seed_packet.go:205`) and the XOR invariant on `plantId`/`newPlant` (`:189-192`). The frontend adding a new path doesn't bypass the service checks. - **File upload is server-validated.** `scanSeedPacket` wraps the body in `http.MaxBytesReader` (30 MiB cap, `seed_packet.go:48`) and runs `imagenorm.Normalize` which rejects unsupported/too-large formats before any model call. The client does no pre-validation, but the server is the authority and aborts oversized streams mid-read. - **No SSRF / secret-leak surface introduced.** The vision call uses a server-configured model spec and `OLLAMA_CLOUD_API_KEY` from the environment (never the DB, per CLAUDE.md); no user-controlled URL is fetched. `sourceUrl` is hardcoded to `''` client-side. `postForm` keeps `credentials: 'same-origin'` and lets the browser set the multipart boundary (no attacker-controllable content-type). - **`PlantIcon` color/icon injection:** the candidates' `color`/`icon` originate from the actor's *own* existing plants (`ListPlantsForActor`), not from the untrusted vision output, and `PlantIcon` is already used elsewhere with the same data — no new vector introduced by this PR. `gadfly-findings` [] </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Confirmed. The finding is real — `ScanPacketModal.tsx:152-153` uses `spacingCm < 1` (cm) but the message says `1 ${unitLabel}`, which in imperial becomes "1 in" while the actual accepted minimum is ~0.39 in. Faithfully replicated from `PlantFormModal.tsx:87-88`. Server `minPlantSpacingCM = 0.1`. ## VERDICT: Minor issues - **`web/src/components/plants/ScanPacketModal.tsx:152-153`** — The spacing validation rejects `spacingCm < 1` (a centimeter threshold) but the error message reads `Spacing must be at least 1 ${unitLabel}`, which in imperial mode becomes "1 in". Re-deriving: 1 in = 2.54 cm, so the real accepted minimum in inches is ~0.39 in, not 1 in. The message therefore *overstates* the floor in imperial — a user who types `0.5 in` (1.27 cm) is accepted while being told they need "1 in". No valid input is wrongly rejected (the cm threshold is stricter than the stated inch one), so this is cosmetic, but it is misleading. This is a faithful copy of the same wording in `PlantFormModal.tsx:87-88`, so it's a pre-existing inconsistency propagated into newly written code rather than a fresh regression. Fix would be to state the threshold in the displayed unit, or simply drop the unit from the message ("Spacing must be a positive number"). Verified against `internal/service/plants.go:19` (`minPlantSpacingCM = 0.1`). Everything else I checked is correct: - `seedPacketSchema`/`packetProposalSchema` defaults match `vision.SeedPacket` nullable fields; unknown `suggestedCategory` falls back to `'vegetable'` via `isPlantCategory`. - `lotDefaults` seed-count → `seeds` unit, else `packets`/qty 1 — matches the backend's `seedLotFields` shape. - Candidate pre-selection (`candidates[0]?.plant.id ?? 'new'`) and the XOR of `plantId`/`newPlant` match `CreateFromPacket`'s "exactly one" rule. - Multipart `FormData` path in `apiFetch` correctly omits the JSON content-type so the browser sets the multipart boundary. - `capabilities.vision` default(false) means an older response hides the button rather than failing the parse (verified `capabilitiesSchema`). - The `onFile` reset-then-read ordering is safe (`file` captured before `e.target.value = ''`). </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All four findings verified against the actual source. Let me confirm the grep claims for shared helpers. All four findings confirmed against the actual source code. No shared helpers exist (`grep` for `validateLot`/`parseYear`/`parseCostCents` returned nothing), and the duplicated constants/messages are verbatim. VERDICT: Minor issues Through the cleanliness/maintainability lens, this is a well-structured change that follows the surrounding patterns (modal shape, mutation hooks, zod schemas in `lib/`, capability gating). The main cleanliness concern is hand-copied logic and magic constants that the comments themselves claim are "the same as" existing code, but which are decoupled rather than shared — so the comments' promises can silently rot. - **`web/src/components/plants/ScanPacketModal.tsx:105-128` — duplicated lot validation.** The `onConfirm` qty / packed-for-year / cost validation blocks are character-for-character copies of `SeedLotModal.tsx:62-88`, and the comment at line 105 explicitly says *"Lot validation mirrors SeedLotModal so the two paths accept the same things."* Verified by reading both files: the `Quantity must be a number, or left blank.` string, `costCents = Math.round(c * 100)`, and the `1900`/`2200` year range appear verbatim in both. There is no shared helper in `lib/seedLots.ts` (confirmed by grep — `validateLot`/`parseYear`/`parseCostCents` don't exist anywhere in `web/src`). This is exactly the "two paths, one invariant, kept in sync by hope" smell: when SeedLotModal's rules change (and they already differ — germination/sourceUrl/purchasedAt/notes are absent from the scan form), the scan path diverges silently. Suggested fix: extract `parseQuantity`/`parsePackedYear`/`parseCostCents` pure functions into `lib/seedLots.ts` and call from both modals; the helpers are also unit-testable, which is where the PR already invests test effort. - **`web/src/lib/seedPacket.ts:96-97` — duplicated default color/icon constants.** `PACKET_PLANT_COLOR = '#4a7c3f'` and `PACKET_PLANT_ICON = '🌱'` are the same values as `DEFAULT_COLOR`/`DEFAULT_ICON` in `PlantFormModal.tsx:22-23` (verified by grep — both files declare these literals, no shared export). They're renamed and re-declared rather than imported, so the two are coupled by intent ("a new plant with no icon gets this placeholder") but decoupled in code — if the manual form's default changes, the packet path's won't follow and the two create paths will produce visually different defaults for the same conceptual "no icon" state. Suggested fix: export `DEFAULT_COLOR`/`DEFAULT_ICON` from a shared spot (e.g. `lib/plants.ts` next to `PlantInput`) and import in both, dropping the `PACKET_PLANT_*` aliases. - **`web/src/lib/seedPacket.ts:98-100` — `DEFAULT_SPACING_CM = 30` claims sameness it can't enforce.** The comment says *"the same default the manual new-plant form uses"*, but `PlantFormModal.tsx:61` inlines the literal `30` (`source?.spacingCm ?? 30`) with no reference to this constant (verified by grep — `DEFAULT_SPACING_CM` appears only in `seedPacket.ts`, not in `PlantFormModal`). So the "sameness" is asserted by prose only; if the manual form's literal changes, the comment becomes a silent lie — the exact "stale comments are worse than none" case CLAUDE.md calls out. Suggested fix: share one `DEFAULT_SPACING_CM` constant (in `lib/plants.ts` or `lib/units.ts`) imported by both, or drop the "same default the manual form uses" claim from the comment. - **`web/src/components/plants/ScanPacketModal.tsx:151-163` — duplicated new-plant field validation.** The `selection === 'new'` branch re-implements spacing / days-to-maturity validation that `PlantFormModal.tsx:86-100` already has (verified: the spacing `< 1` check with the same `Spacing must be at least 1 ${unitLabel}.` message and "Days to maturity must be a whole number of days" are copied verbatim). Less severe than the lot duplication because the field sets aren't identical (the packet form has no icon/color/sourceUrl/notes, and the name-required message differs), but the spacing + days checks are candidates for a shared `validatePlantCoreFields(...)` helper if the lot helper above is also extracted. Noting it as the same pattern, not a separate must-fix. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I've verified the key paths against the checked-out code. Performance lens: - `useScanPacket`/`useCreateFromPacket` are standard one-shot mutations; no caching concerns (intentionally uncached, each photo is fresh). - The scan uploads the raw photo with no client-side downscaling. This is the dominant cost of the flow (multi-MB phone photo → 60s read timeout was added server-side precisely for this), but it's a deliberate architecture choice: HEIC can only be decoded server-side (`imagenorm`), and normalization is centralized there. Not a regression introduced by this PR, and client-side pre-resize would only help the JPEG/WebP subset. Not flagging. - `categoryOptions`/`unitOptions` are module-level constants, not rebuilt per render — good. - `proposal.candidates` is bounded server-side (`maxPlantMatches = 10` in `internal/service/plants.go:91`), so the `.map` in the review phase is bounded. Verified. - The 11 `setState` calls in `scan.mutate.onSuccess` all fire inside one event-handler callback, so React 18 auto-batches them into a single re-render. Fine. - `useCreateFromPacket.onSuccess` invalidates `plantsKey` and `seedLotsKey` — both queries already run on `PlantsPage`, so the refetch is the expected, necessary cost of showing the new rows. No over- or under-invalidation. - `ReadFields` builds a small `rows` array per render (≤5 entries); trivial. - `useCapabilities()` on `PlantsPage` adds one fetch, cached with `staleTime: 60_000`; not a hot path. No efficiency regressions, unbounded growth, N+1, or avoidable hot-loop work in this change. ## VERDICT: No material issues found (Performance lens clean; verified the candidate cap, the invalidation targets, and that the heavy image payload is a pre-existing server-side-normalization design decision, not a new regression.) </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I'll investigate the PR through the error handling & edge cases lens. Let me verify key areas against the actual repo. Now let me verify the `PlantInput`/`seedLot`/`LotInput` shapes to check the `lot` object built in `ScanPacketModal` against the actual `SeedLotInput` and `PlantInput`. Let me check `cmFromSpacing` and `spacingFromCm` to verify NaN handling, and the backend seed-packet endpoints for what shapes they actually accept. `parseFloat('')` returns `NaN`, and `cmFromSpacing(NaN, ...)` returns `roundCm(NaN*...)`. Let me check `roundCm`: `cmFromSpacing(NaN, unit)` → `roundCm(NaN)` → `NaN`. Then `!Number.isFinite(spacingCm) || spacingCm < 1` catches NaN. Good. Now let me verify the backend endpoint to understand what happens with `newPlant` field ordering / extra fields, and whether the `lot` object's fields match what's accepted. Let me also check whether `purchasedAt: null` and other nulls are acceptable server-side. Now let me verify the API handler's binding shape for `from-packet` — particularly whether `plantId` (lowercase d) in the JSON is decoded correctly given the service uses `PlantID`. Now let me verify the `seedLotFields` server-side to check the `lot` object built in the modal matches (esp. `purchasedAt: null` vs optional string), and `plantCreateRequest` to check `newPlant` field shape (e.g. `daysToMaturity` null handling, `sourceUrl` etc.). Confirmed: server side `purchasedAt *string`, `costCents *int`, `germinationPct *float64`, `packedForYear *int`, `daysToMaturity *int`. The modal sends `purchasedAt: null`, `costCents` (number|null), `germinationPct: null`, `packedForYear` (number|null), `daysToMaturity` (number|null). All decode fine into Go pointers/nullable. Now let me look at one edge case in the modal: the `cmFromSpacing(parseFloat(spacing), unit)` when `spacing` is empty string. In the `selection === 'new'` path, validation catches NaN. Good. Let me also double-check: `onFile` resets `e.target.value = ''` then reads `e.target.files?.[0]` afterward. The reset happens before reading the file — does clearing `.value` also clear `.files`? The `e.target.value = ''` after grabbing `file` is a well-known safe pattern — `file` holds the File reference, reset of `.value` just clears the input. That's fine. Now let me consider an important edge case in the error-handling lens: what if `scan.mutate` is called while a previous scan is pending (rapid double-pick)? React Query mutations aren't queued by default; `mutate` while pending — default `mutationKey` none — it'll just start a new mutation. Could cause two proposals landing and the second `onSuccess` racing. But this is minor and not a crash. Also, the file input is hidden and only triggered when `!scan.isPending`, so the button itself is gated. Let me check the `ReadFields` rendering with a NaN edge case — `spacingFromCm(p.spacingCm, unit)` where `p.spacingCm` could be 0 or negative from a bad model read. The schema allows any number. Let me check whether negative spacing could slip through. Now let me think about edge cases more carefully: 1. **`spacingFromCm(p.spacingCm, unit)` in `ReadFields` (line 386 of modal)**: if `p.spacingCm` is negative or 0, this renders a weird value. But it's read-only display — not a crash, minor. 2. **`newPlantDefaults` spacing**: `p.packet.spacingCm ?? DEFAULT_SPACING_CM` — if model returns `spacingCm: 0` (printed "0"), prefill becomes 0. Then in review phase, user editing "new" path: `cmFromSpacing(parseFloat(spacing), unit)` where spacing="0" → `spacingCm=0` → `!Number.isFinite(0) || 0 < 1` → `0 < 1` true → error "Spacing must be at least 1". That's caught. But the default prefilled value 0 would fail validation if user just clicks submit unchanged. Actually it would show the error — that's handled, not a crash. Acceptable. 3. **Quantity**: `Number('')` is 0, but `qty = quantity.trim() === '' ? 0 : Number(quantity)`. `Number('abc')` is NaN, caught. `Number('1e308')` etc. fine. `Number('-5')` → -5, caught by `< 0`. Good. But `Number('Infinity')` → Infinity, `Number.isFinite(Infinity)` false → error. Good. 4. **Cost**: `Number(cost)` then `Math.round(c * 100)`. `c` could be huge. `Number.MAX_SAFE_INTEGER * 100` overflow not really an issue for typical currency. Fine. 5. **`packedForYear`**: `Number(packedForYear)` — `Number('2026.5')` = 2026.5, `Number.isInteger(2026.5)` false → error. Good. `Number('1e4')` = 10000, integer, in range. Edge: scientific notation accepted but unusual; fine. Now the **main edge case I want to flag**: In `onConfirm`, the existing-plant path (`selection !== 'new'`) sets `plantId = selection` but does NOT validate that this plant id still exists/is visible. The backend handles that (`visiblePlant` → ErrNotFound). But more importantly, when `selection` is a candidate plant id, and `newPlant` is left undefined — good, exactly-one satisfied. Backend rejects both/neither. Fine. But there's a subtle issue: **if `proposal.candidates` is empty**, `setSelection('new')` and the radio shows "Add as a new variety". Good. If user selects an existing candidate then the form's name/spacing fields are hidden (only shown when `selection === 'new'`). On submit with existing selection, `newPlant` undefined, `plantId` set. Good. Now, one real concern: **`cmFromSpacing(parseFloat(spacing), unit)` only happens in the `selection === 'new'` branch.** Good — existing-plant path doesn't need spacing. Fine. Let me check whether there's an issue with the `cost` field being non-finite but the `Math.round(c * 100)` producing NaN that slips to backend. No — `!Number.isFinite(c) || c < 0` guards before `Math.round`. Good. Let me verify the `errorMessage` for the 503 VISION_DISABLED case actually surfaces a usable message (the backend writes `error.message`). The `messageFrom` reads `body.error.message`. Good. The `onError` fallback is only used if not an ApiError. Fine. Let me check one more: the `scan.mutate` `onError` — does React Query call `onError` for a zod parse failure inside `mutationFn`? The `packetProposalSchema.parse` throws a ZodError, which propagates out of `mutationFn` → React Query treats as mutation error → `onError` fires with the ZodError. `errorMessage(ZodError)` → not ApiError → fallback "Couldn't read that photo...". That's a misleading message for a parse failure (server returned unexpected shape), but it degrades gracefully rather than crashing. Minor. Now let me look at the `PlantsPage` integration to verify the `capabilities.data?.vision` gate handles `capabilities` still loading / error. The `capabilities.data?.vision` optional-chain handles `data` being undefined (loading/error) → falsy → button hidden. Good. The `vision` default(false) ensures older backends hide it. Good. Now let me check one potential real issue: when `capabilities` query errors (network), `data` is undefined, button hidden — fine, no crash. When the query is stale and shows old `vision:true` then backend disabled vision — the modal's scan would 503, handled via `onError`. Fine. Let me check the `Modal` `busy` prop behavior — does `busy` block the close (X) button? If a scan is pending and user hits Escape or backdrop, does it cancel? Let me check Modal. `Modal` `busy` blocks Escape and backdrop close. Good — so during a scan the modal stays. The Cancel buttons are `disabled={busy}`. Good. The "Rescan" button (sets `proposal(null)`) is `disabled={busy}` — good, can't rescan mid-scan. But wait — there's a subtle edge: `busy = scan.isPending || create.isPending`. During review phase, `scan.isPending` is false (scan done) and `create.isPending` is false until submit. So `busy` is false in review — fine. During capture, `scan.isPending` true → busy true. Good. One thing: in the **review phase**, the hidden file input is not rendered (only in capture phase). So user can't re-pick a photo while reviewing — they must click "Rescan" to go back to capture. Fine, that's intentional. Now let me check whether `scan.mutate` being called when a previous `create` is pending could interleave. The capture phase only shows when `!proposal`, and `proposal` is set on scan success. Once in review, capture input not shown. So no double-scan. Fine. Let me check the `cost` edge: `cost.trim()` then `Number(cost)` (not `cost.trim()`). Minor inconsistency but `Number(' 4.99 ')` = 4.99, fine. Actually they check `cost.trim()` for emptiness but pass `cost` (untrimmed) to Number — Number trims whitespace anyway. Fine. Now let me look at the most important potential real issue from the error-handling lens: **`setPackedForYear(lot.packedForYear != null ? String(lot.packedForYear) : '')`** — `lot.packedForYear` is `number | null` from `lotDefaults`. If it's a number, `String(number)`. Fine. But `lotDefaults` returns `packedForYear: p.packedForYear` where `p.packedForYear` is `number | null`. Good. Let me verify the `spacing` default display: `setSpacing(String(spacingFromCm(plant.spacingCm, unit)))`. `plant.spacingCm` from `newPlantDefaults` is `p.packet.spacingCm ?? DEFAULT_SPACING_CM` → always a number. `spacingFromCm` returns a number. `String(number)`. If spacingCm is e.g. 30 → "30" metric, or imperial 30cm → 11.8 → "11.8". Fine. If `spacingCm` were NaN somehow, `String(NaN)` = "NaN", and on submit `parseFloat("NaN")` = NaN → caught. Acceptable. Now let me check the test file briefly to confirm test coverage claims and that tests don't assert wrong things. The tests look reasonable. Let me focus on one more potential real issue: **the `lot` object sent on confirm is missing nothing required, but `unit` validation.** Backend `seedLotFields.Unit` has `binding:"required"`. The modal always sends `lotUnit` (default 'packets'), so always present. Good. Let me check whether there's an issue with `germinationPct: null` — backend `GerminationPct *float64` accepts null. The `SeedLotInput` TS type requires `germinationPct: number | null`. Modal sends `null`. Good. Let me verify the `SeedLotFields` type matches what modal builds (TypeScript-level). `SeedLotFields = Omit<SeedLotInput, 'plantId'>`. `SeedLotInput` has: vendor, sourceUrl, sku, lotCode, purchasedAt, packedForYear, quantity, unit, costCents, germinationPct, notes. Modal builds exactly these. Good. Now, one genuine concern from the error-handling lens: **`Number(quantity)` when `quantity` is something like `" "` (whitespace only)** — `quantity.trim() === '' ? 0 : Number(quantity)`. `" ".trim()` = "" → 0. Good. But `Number(" 5 ")` = 5. Good. `Number("5abc")` = NaN → caught. Good. Let me check: **`Number(days)` for daysToMaturity** — `days.trim()` then `Number(days)`. `Number("90")` = 90. `Number("90.5")` = 90.5, `Number.isInteger(90.5)` false → error. Good. `Number("")` would be 0 but guarded by `days.trim()`. Good. I think the error handling is quite solid. Let me do a final check on one thing: the `onConfirm` is `async` and uses `await create.mutateAsync(...)`. If the component unmounts mid-await (user closes modal? No — busy blocks close). Actually `onClose()` is called inside the try after success. During pending, busy true blocks backdrop/escape. The Cancel/Rescan buttons disabled. So no unmount-during-await. Good. No setState-after-unmount risk in the success path. The `catch` sets error — component still mounted. Fine. Let me check one last potential: **`create.mutateAsync` throwing an `ApiError` with `isConflict` (409)** — possible if the candidate plant was version-conflicted? Actually creating a lot against an existing plant doesn't carry version. Creating a new plant doesn't either. So 409 unlikely. But if it happens, `errorMessage` surfaces the message. No special conflict-rebase handling like other forms. That's an inconsistency vs SeedLotModal which calls `conflictSeedLot`. But for a *create* there's no version to rebase. Minor, not a crash. Let me verify SeedLotModal doesn't do conflict rebase on create either (to confirm this isn't a regression). That conflict rebase is only relevant for *edit* (PATCH with version). The packet confirm is a *create* (no version), so 409 won't occur on the lot create itself. Confirmed not a regression. I've thoroughly checked the error-handling and edge cases. The code is notably defensive: NaN guards, empty-string handling, null/optional server fields, the `vision` default(false) for older backends, busy-lock preventing close/interleave, and the exactly-one validation delegated to the backend. The unhappy paths (503 VISION_DISABLED, unreadable image, too-large) are all funneled through `errorMessage` with sensible fallbacks. The only minor items I found: 1. A Zod parse failure on the scan response (server returned an unexpected shape) surfaces the generic "Couldn't read that photo…" fallback rather than something indicating a server/protocol mismatch — misleading but not a crash, and an unlikely path given the schema is shared. 2. `spacingFromCm(p.spacingCm, unit)` in `ReadFields` will render a nonsensical value if the model returns a negative/zero `spacingCm`, but it's a read-only display and the model is instructed to leave absent fields null (so 0/negative is unlikely). Not a crash. Neither rises to "material" for the error-handling lens. The validation, null-handling, and unhappy-path coverage are solid. ## Verdict: No material issues found Through the error-handling & edge-cases lens, the change is solid. Verified by reading the files: - **NaN / empty / negative guards** in `onConfirm` (`ScanPacketModal.tsx:106-119`, `:127-135`, `:138-144`): `quantity`, `packedForYear`, `cost`, `spacing`, `days` all trim-check emptiness, then `Number.isFinite`/`Number.isInteger`/range checks before use. `cmFromSpacing(parseFloat(spacing), unit)` producing NaN is caught by `!Number.isFinite(spacingCm) || spacingCm < 1` (`:128`). Confirmed `roundCm`/`cmFromSpacing` propagate NaN (units.ts:29-31, 191-193), so the guard is load-bearing and correct. - **Null/optional server fields**: the `lot` object (`:121-134`) and `newPlant` (`:146-154`) match `seedLotFields` (seed_lots.go:24-36) and `plantCreateRequest` (plants.go:16-26) — nullable Go pointers accept the `null`s the client sends (`purchasedAt`, `costCents`, `germinationPct`, `packedForYear`, `daysToMaturity`). Verified the TS `SeedLotFields = Omit<SeedLotInput,'plantId'>` and the modal builds exactly those keys. - **`vision` default(false)** (agent.ts) + `capabilities.data?.vision` gate (PlantsPage.tsx:81): handles loading/error/older-backend gracefully — optional chain yields falsy, button hidden, no dead button, no parse failure. Verified against the schema. - **Busy-lock** prevents interleaving/close-during-pending: `busy = scan.isPending || create.isPending` flows to `Modal busy` which blocks Escape/backdrop (Modal.tsx:47,94), and the Cancel/Rescan buttons are `disabled={busy}` (`:319`,`:323`). No setState-after-unmount in the async `onConfirm` success path (onClose only after await). - **Scan error funneling**: 503 `VISION_DISABLED` / unreadable / too-large all return `error.message` via `messageFrom` (api.ts:80-88), surfaced through `errorMessage(err, fallback)` (`:95-96`). A non-`ApiError` (e.g. a zod parse failure inside `mutationFn`) falls back to the generic message — degrades gracefully, no crash. - **Exactly-one validation** (plantId XOR newPlant) is enforced server-side (seed_packet.go:189-192); the client always sends exactly one (`selection==='new'` → newPlant, else → plantId), so neither "both" nor "neither" can occur. - **`e.target.value = ''` after grabbing `file`** (`:70-73`): the File reference is retained; resetting the input only allows re-picking the same file. Correct pattern. Two non-material observations (not blocking, no crash/incorrect behavior): - A Zod parse failure on the scan response surfaces the "couldn't read that photo" fallback rather than a protocol-mismatch indication — unlikely given the shared schema, and it degrades safely. - `ReadFields` renders `spacingFromCm(p.spacingCm, unit)` for any non-null `spacingCm`; a pathological 0/negative model read would show a nonsensical read-only value, but the prompt instructs the model to leave unprinted fields null, and confirm-path validation re-checks the editable spacing. </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** Through the Security lens I traced every tainted-data path in the diff: - **File upload** → `FormData` → `api.postForm` → backend `http.MaxBytesReader` → `imagenorm.Normalize` (JPEG re-encode). No filename-based path injection; content is normalized server-side before reaching the vision model. - **Frontend form inputs** (`name`, `vendor`, `sku`, `lotCode`, `quantity`, `cost`, `spacing`, `days`) are validated (trim, numeric checks, range bounds) before submission. `parseFloat`/`Number` on empty strings produce `NaN`, which the existing `!Number.isFinite(...)` guard rejects. - **Candidate `plant.id` selection** is an integer validated by `plantSchema` on arrival; no path-traversal or injection vector. - **Capabilities schema** now defaults both `agent` and `vision` to `false` — a secure-by-default improvement that hides features on partial/older responses instead of exposing them or crashing the parse. - **React rendering** of all user-supplied strings (proposal fields, form inputs, error messages) goes through React’s normal JSX escaping; no `dangerouslySetInnerHTML` is introduced. - **Error messages** from the backend (`ApiError`) are user-facing static strings in the Go handlers; no stack-trace or internal-path leakage. - **Secrets** remain untouched; `OLLAMA_CLOUD_API_KEY` stays env-only per DESIGN.md, and the UI only sees a boolean `vision` capability. Nothing in this PR introduces an auth/authz gap, injection surface, SSRF, deserialization risk, secret leak, or unsafe default. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I reviewed the seed-packet scan UI change through the **correctness** lens, tracing every validation rule, type boundary, and backend contract. Here's what I checked: **Validation logic in `ScanPacketModal.tsx`** — The `onConfirm` handler mirrors `SeedLotModal` exactly: quantity ≥ 0, year 1900–2200, cost ≥ 0, spacing ≥ 1 cm, days ≥ 1 (or null). All edge cases (empty string, NaN, negative, zero) are handled correctly. `parseFloat(spacing)` → `cmFromSpacing` → `!Number.isFinite` correctly rejects non-numeric and out-of-range inputs. **Unit conversions** — `spacingFromCm` / `cmFromSpacing` round-trip correctly for both metric and imperial. The `roundCm` dust-guard (1e-6) is applied consistently. **Backend contract alignment** — `FromPacketBody` sends exactly one of `plantId` (number) or `newPlant` (`PlantInput`), matching the Go struct `fromPacketRequest` which demands exactly one via `PlantID *int64` XOR `NewPlant *plantCreateRequest`. The lot shape (`SeedLotFields = Omit<SeedLotInput, 'plantId'>`) matches `seedLotFields.toInput()` on the server. **Schema defaults** — `seedPacketSchema` defaults every field safely. `newPlantDefaults` falls back to `'vegetable'` for unknown categories (defensive against model hallucination) and `30` cm for missing spacing, matching the manual new-plant form. `lotDefaults` correctly interprets a positive `seedCount` as seeds and defaults to 1 packet otherwise. **FormData upload** — `api.postForm` passes `FormData` through unchanged with no JSON content-type header, letting the browser set the multipart boundary. The `instanceof FormData` guard in `apiFetch` is safe and backward-compatible. **Capabilities schema** — Adding `vision: z.boolean().default(false)` alongside `agent` means older/partial responses hide the feature rather than failing parse. This is the correct defensive choice. **Query invalidation** — `useCreateFromPacket` invalidates `plantsKey` and `seedLotsKey` on success, so the catalog refreshes after confirm. Everything checks out. The prefill logic, validation, and backend contracts are all consistent. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`web/src/components/plants/ScanPacketModal.tsx:106`** — The quantity, packed-for-year, and cost validation blocks are copy-pasted from `SeedLotModal.tsx:62-88` (same messages, same logic). The PR description even calls this out ("mirrors SeedLotModal"). Extract shared validators (e.g. `validateLotQuantity`, `validateYear`, `validateCost`) so a rule change doesn't need two updates. - **`web/src/components/plants/ScanPacketModal.tsx:26`** — `categoryOptions` is identical to the one in `PlantFormModal.tsx:25`. It should be exported once from `@/lib/plants` (e.g. `export const categoryOptions = …`) and imported by both modals. - **`web/src/components/plants/ScanPacketModal.tsx:27`** — `unitOptions` duplicates the inline expression in `SeedLotModal.tsx:167`. It should be exported once from `@/lib/seedLots` (e.g. `export const lotUnitOptions = …`) and shared. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found. The frontend-only change is a well-contained modal dialog with local form state. From a performance lens: - **Image upload**: No client-side size limiting or compression before the multipart `POST`, so a modern phone camera's full-resolution JPEG (~2–5 MB) travels uncompressed. This is worth keeping in mind for mobile bandwidth and server memory, but it's a UX/API design choice inherited from the already-live backend (#94), not a new frontend regression introduced by this diff. - **Query invalidation**: `useCreateFromPacket` broadly invalidates `plantsKey` and `seedLotsKey`. These are simple unparameterized keys and match the existing invalidation patterns elsewhere in the codebase (e.g. `plants.ts`, `seedLots.ts`), so there's no new cache thrashing. - **Rendering**: The candidate list renders directly from the backend response. `matchPlants` on the backend doesn't cap results, but its matching logic (exact name → variety-in-name → same species word) is conservative enough that runaway lists are unlikely in practice, and this is pre-existing backend behavior. Nothing in the changed code introduces N+1 queries, quadratic loops, unnecessary allocations inside hot paths, or blocking work on render. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict: No material issues found** I scrutinized the diff specifically through the error-handling and edge-cases lens. Here is what I checked and why it is clean: - **Nil / empty inputs:** Every form field that can be left blank is guarded (`quantity.trim() === '' ? 0`, `cost.trim()`, `days.trim()`, `packedForYear.trim()`). The file-input reset (`e.target.value = ''`) is done before the early return so re-selecting the same file after an error always fires `onChange`. - **Numeric validation:** `Number.isFinite`, `Number.isInteger`, and range checks (`< 1`, `< 0`, `< 1900`, `> 2200`) are applied to every parsed field. `parseFloat('')` → `NaN` is caught by `!Number.isFinite`. `Number('abc')` → `NaN` is likewise caught. - **Spacing edge cases:** `cmFromSpacing(parseFloat(spacing), unit)` is validated with `!Number.isFinite(spacingCm) || spacingCm < 1`, which catches `NaN`, `Infinity`, negative values, and zero/negative-zero. - **Overflow:** `costCents = Math.round(c * 100)` could theoretically exceed `Number.MAX_SAFE_INTEGER` for absurd inputs, but this is a server-validated field and the client guards (`Number.isFinite`, `c < 0`) are appropriate for the UI layer. - **Abort / unmount during async work:** `busy = scan.isPending || create.isPending` is piped into the `Modal`’s `busy` prop, which disables the backdrop-click and Escape close paths as well as the Cancel/Rescan buttons. The only remaining unmount path is a full page navigation; in React 18 state updates on an unmounted component are silently discarded, so this does not produce a crash or leak. - **Schema fallback safety:** `capabilitiesSchema` defaults both `agent` and `vision` to `false`, so an older/partial server response hides the feature rather than failing parse. `seedPacketSchema` defaults every field, and `newPlantDefaults` falls back unknown categories to `'vegetable'` and missing spacing to `30` cm. - **FormData multipart path:** The `instanceof FormData` check in `apiFetch` correctly omits the JSON content-type so the browser generates the multipart boundary. `postForm` is a thin, typed wrapper and is used only in the browser. Nothing in this diff ignores errors, swallows exceptions, leaks resources, or mishandles boundary conditions in a way that reaches the user or breaks the UI. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-22 17:06:25 +00:00
Address review: rescan staleness + escapable slow scan
Build image / build-and-push (push) Successful in 11s
d9c2de4882
From the Gadfly review of #119:

- Reset the cost field on a new scan — it was the one review field not reseeded
  in the success handler (there's no cost on a packet), so a value typed before a
  Rescan lingered into the next proposal.
- Clear the review-phase error when returning to capture via Rescan, so a failed
  confirm doesn't leave a stale error showing on the capture screen.
- Make the scan escapable: thread an AbortController through the scan mutation and
  wire it to Cancel (no longer disabled while scanning), so a slow or hung vision
  call — the server allows up to 120s — isn't a trap the user can only leave by
  reloading. An aborted scan is treated as a cancel, not an error.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve merged commit cf37e57808 into main 2026-07-22 17:07:26 +00:00
steve deleted branch feat/seed-packet-scan-ui 2026-07-22 17:07:26 +00:00
Sign in to join this conversation.