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.
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
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
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>
🪰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>
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.
🪰 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>
⚪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>
🔴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>
🟠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>
🟠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>
🟡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>
🟡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>
🟡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>
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 detailclaude-code/sonnet (claude-code) — Minor issues
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.
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
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.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.
🧯 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.
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-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.
⚡ 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:
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.
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.
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.
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.
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.mutateonError — 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 Modalbusy 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. Modalbusy 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:
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.
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
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.
🎯 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 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.
🧹 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: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.
⚡ 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>
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 main2026-07-22 17:07:26 +00:00
steve
deleted branch feat/seed-packet-scan-ui2026-07-22 17:07:26 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #102. The last open child of epic #96.
The seed-packet capture backend has been live since #94 —
POST /seed-lots/scan(multipart image → aPacketProposal) andPOST /seed-lots/from-packet(confirm → plant + lot), with/capabilitiesreportingvision— 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
capabilities.visionis on — never a dead button on an instance with no vision model.visionis added to the capabilities schema (both flags nowdefault(false), so a partial/older response hides a feature rather than failing the parse).ScanPacketModal— two phases in one dialog: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.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.tslearns to sendFormDataas multipart (no JSON content-type) for the upload, via a newapi.postForm.Frontend only; no backend or route change.
Testing
tscclean, 108 web tests green (7 new for the schemas + prefill),npm run buildclean (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
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 17:03:19Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 10 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -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:26—categoryOptionsis identical to the one inPlantFormModal.tsx:25. It should be exported once from@/lib/plants(e.g.export const categoryOptions = …) and imported by both modals.🪰 Gadfly · advisory
@@ -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
@@ -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— Thecostform state is never reset when a new packet is scanned.onFilesetsname,category,spacing,days,vendor,quantity,lotUnit,sku,lotCode, andpackedForYearfrom the new proposal, butsetCost('')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
@@ -0,0 +102,4 @@if (!proposal) returnsetError(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. TheonConfirmqty / packed-for-year / cost validation blocks are character-for-character copies ofSeedLotModal.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: theQuantity must be a number, or left blank.string,costCents = Math.round(c * 100), and the1900/2200year range a…🪰 Gadfly · advisory
@@ -0,0 +127,4 @@costCents = Math.round(c * 100)}const lot = {🟡 Hand-built
lotliteral re-spells SeedLotFields instead of spreading lotDefaults + overridesmaintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -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 usesparseFloat(spacing), whilequantityanddaysuse the stricterNumber()(lines 106, 149).parseFloataccepts trailing garbage (e.g."15abc"→15), so malformed spacing input can slip through where other fields would be rejected. Since the input istype="number"this is harder to trigger in practice, but the inconsistency weakens validation. Fix: replaceparseFloat(spacing)with `Number(spacin…🪰 Gadfly · advisory
@@ -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. Theselection === 'new'branch re-implements spacing / days-to-maturity validation thatPlantFormModal.tsx:86-100already has (verified: the spacing< 1check with the sameSpacing 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
@@ -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(withModal's blocking logic atweb/src/components/ui/Modal.tsx:47,94): while a scan is in flight, there is no way to cancel or bail out.Modaldisables Escape (Modal.tsx:47, gated on!busyRef.current) and backdrop-click-to-close (Modal.tsx:94, gated on!busy) wheneverbusyis true, andScanPacketModal's own capture-phase Cancel button isdisabled={busy}(ScanPacketModal.tsx:227). Confirmed `busy = scan.isPending || crea…🪰 Gadfly · advisory
@@ -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
@@ -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'andPACKET_PLANT_ICON = '🌱'are the same values asDEFAULT_COLOR/DEFAULT_ICONinPlantFormModal.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
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 10 findings (3 with multi-model agreement)
web/src/components/plants/ScanPacketModal.tsx:105web/src/components/plants/ScanPacketModal.tsx:363web/src/components/plants/ScanPacketModal.tsx:1517 single-model findings (lower confidence)
web/src/components/plants/ScanPacketModal.tsx:88web/src/components/plants/ScanPacketModal.tsx:143web/src/components/plants/ScanPacketModal.tsx:26web/src/components/plants/ScanPacketModal.tsx:130web/src/components/plants/ScanPacketModal.tsx:191web/src/lib/seedPacket.ts:96web/src/components/plants/ScanPacketModal.tsx:47Per-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 theFormDatabranch never sets a manualcontent-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 —pathvalues passed toapi.postForm/api.postare 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, nodangerouslySetInnerHTMLor similar sink.selection/plantIdis constrained tonumber | 'new'and the numeric branch only ever comes fromc.plant.idin 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 badsuggestedCategorycan'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-sidecapabilities.visioncheck is purely a UX gate (hide vs. show a button); it grants no access itself since the server independently gates the vision call.agent/visionnow.default(false)) fails safe: a partial/malformed/capabilitiesresponse 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:
seedPacketSchema/packetProposalSchema/packetResultSchema(web/src/lib/seedPacket.ts) field-by-field againstvision.SeedPacket,service.PacketProposal/PacketConfirm/PacketResult, and thefromPacketRequest/plantCreateRequest/seedLotFieldsJSON tags ininternal/api/{seed_packet,plants,seed_lots}.go— names and optionality match.cmFromSpacing/spacingFromCm(web/src/lib/units.ts:141-152) andDEFAULT_SPACING_CM = 30match the existingPlantFormModalconventions exactly (same default, samespacingCm < 1floor, same round-trip viaroundCm).Selection = number | 'new'againstplantSchema.id: z.number()(web/src/lib/plants.ts) — no type mismatch, and the?? 'new'(not||) correctly treats a candidate id of0as valid.plantId/newPlantare mutually exclusive when serialized (JSON.stringify dropsundefined), matching the backend'shasID == hasNewXOR check inCreateFromPacket(internal/service/seed_packet.go:189-192).isPlantCategory/PLANT_CATEGORIESmatches the backend'splantCategoriesset used byvalidCategory(vegetable/herb/flower/fruit/tree_shrub/cover) — an unknown/model-hallucinated category can't slip through.apiFetch's new FormData branch omits the JSON content-type header (letting the browser set the multipart boundary) and that the field nameimagematchesc.FormFile("image")server-side.Modal'sbusyflag 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.tsxmatchSeedLotModal.tsx:62-88exactly (same conditions, same error strings, same 1900/2200 bounds), and lines 151-164 matchPlantFormModal.tsx:86-100exactly (same spacing/days parsing and error strings). The draft's finding is accurate.Verdict: Minor issues
web/src/components/plants/ScanPacketModal.tsx:106-127duplicates the quantity/packedForYear/cost validation verbatim fromweb/src/components/plants/SeedLotModal.tsx:62-88(identical conditions, error strings, and the1900/2200year bounds — confirmed by direct comparison of both files), andScanPacketModal.tsx:151-164separately duplicates the spacing/days-to-maturity validation fromweb/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.SeedLotModalandPlantFormModal, 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
postFormaddition to the API client). Checked for the usual suspects:proposal.candidatesand the various form arrays (PLANT_CATEGORIES,LOT_UNITS) are small, fixed-size, module-scope constants (verified inweb/src/lib/plants.ts:10andweb/src/lib/seedLots.ts:13-20), not recomputed per render.useScanPacket) is correctly a one-shot, uncached mutation — appropriate since each photo is unique; no risk of stale-cache growth.useCreateFromPacket'sonSuccessinvalidates exactlyplantsKeyandseedLotsKey(web/src/lib/seedPacket.ts:66-69) — matches the two catalogs actually affected, no over-invalidation cascading unrelated queries.api.ts(web/src/lib/api.ts:98-101) passes theFormDatathrough 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 inonFile, once again inonConfirm), but it's a cheap plain-object construction on a small, already-parsed proposal — not a measurable cost.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(withModal's blocking logic atweb/src/components/ui/Modal.tsx:47,94): while a scan is in flight, there is no way to cancel or bail out.Modaldisables Escape (Modal.tsx:47, gated on!busyRef.current) and backdrop-click-to-close (Modal.tsx:94, gated on!busy) wheneverbusyis true, andScanPacketModal's own capture-phase Cancel button isdisabled={busy}(ScanPacketModal.tsx:227). Confirmedbusy = 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 neitheruseScanPacket'smutationFn(web/src/lib/seedPacket.ts:70-78,async (file: File), no signal threaded through) norapiFetchis ever invoked with anAbortSignalfor this call, even thoughRequestOptions.signalexists 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 likeSeedLotModal/PlantFormModal. Suggested fix: wire anAbortControllerinto 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:dangerouslySetInnerHTML: grep confirms none inweb/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. VerifiedAlert.tsxrenders children as plain ReactNode, not raw HTML.useScanPacketparses viapacketProposalSchema(and nestedplantSchema/seedPacketSchema) before the data reaches state;useCreateFromPacketparses the result viapacketResultSchema. Unknown category is explicitly neutralized to'vegetable'viaisPlantCategoryrather than passed through — a malformed/attacker-influenced model response can't smuggle an arbitrary category intoPlantInput.api.postFormlets the browser set the multipart boundary; the diff correctly omits the JSON content-type forFormData(api.ts:99-112). No header injection / boundary-clobber path.qty,yearrange,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.credentials: 'same-origin'(session cookie) same as every other call; no new auth surface introduced. Gating the entry point oncapabilities.visionprevents 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 isspacingCm < 1wherespacingCm = cmFromSpacing(parseFloat(spacing), unit)(cmFromSpacingreturnsvalue * CM_PER_INCHfor imperial,CM_PER_INCH = 2.54, atweb/src/lib/units.ts:191-203,7), so the real floor is 1 cm. The message reads`Spacing must be at least 1 ${unitLabel}.`andspacingUnitLabelreturnsinfor 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 entering0.5in (≈1.27 cm) passes despite being told the minimum is 1 in, and0.4in (≈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 againstunits.ts:191-203and 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 —
isPlantCategoryfollows 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.') plusMath.round(c * 100)are copy-pasted fromSeedLotModal.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 pureparseLotFields(...)helper returning{ values, error }intolib/seedLots.tsand call it from both modals. Low churn, one source of truth.ScanPacketModal.tsx:130–142— hand-builtlotobject re-spellsSeedLotFields. The literal sets every field ofSeedLotFields(seedPacket.ts:57), hard-codingsourceUrl: '',purchasedAt: null,germinationPct: null,notes: ''to the exact valueslotDefaultsalready returns (seedPacket.ts:131–147); the only deltas are the user-editedquantity/unit/costCents/packedForYear/sku/lotCode/vendor.lotDefaultsis already imported and used in the prefillonSuccess, 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— 14useStateslots in one ~330-line component. The component mixes capture, prefill, validation, and confirm in one body, and the prefillonSuccess(lines ~78–93) scattersnewPlantDefaults/lotDefaultsresults across a wall of setters. Readable now, but auseReduceror singlereviewStateobject 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
Filein React state (transient, GC'd after the mutation),ReadFieldsbuilds 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.postFormcorrectly avoidsJSON.stringifyand lets the browser stream the multipart body.useScanPacketis 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 nosetError(null); the sharederrorstate 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 (onFileat line 75). The draft cited line 284, but the actual Rescan button is at line 363 — correcting that.Verdict: Minor issues
ScanPacketModal.tsx:363— the Rescan button's handler is() => setProposal(null)with nosetError(null). If a confirm attempt failed (e.g. server rejected the lot),errorholds "Could not save the packet." (set atScanPacketModal.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 (becauseonFilecallssetError(null)atScanPacketModal.tsx:75).erroris one shared state variable rendered in both phases. Suggested fix: clearerrorin the Rescan handler, e.g.onClick={() => { setProposal(null); setError(null) }}.No other material error-handling issues: the scan
onErrorcorrectly surfaces the server message, the confirm try/catch wraps only the awaited mutation (validation failuresreturnrather than throw),void qc.invalidateQueriesis the intended fire-and-forget, and the null/empty guards inReadFields(p.spacingCm != null,p.seedCount != null) andlotDefaults(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:
api.postFormcorrectly omits thecontent-typeheader forFormData, letting the browser generate the multipart boundary. No path traversal or injection risk — theFileobject is passed as-is.species,variety, etc.) is rendered as React text nodes inReadFieldsand form inputs — nodangerouslySetInnerHTML, no HTML injection surface. Toast and Alert components also render text, not HTML./seed-lots/scanand/seed-lots/from-packetare parsed through Zod schemas (packetProposalSchema,packetResultSchema), preventing malformed data from propagating.capabilities.visionis false, preventing 404/503 exposure.The changes are clean from a security perspective.
🎯 Correctness — Blocking issues found
Blocking issues found
web/src/components/plants/ScanPacketModal.tsx:88-95— Thecostform state is never reset when a new packet is scanned.onFilesetsname,category,spacing,days,vendor,quantity,lotUnit,sku,lotCode, andpackedForYearfrom the new proposal, butsetCost('')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: addsetCost('')in theonFilesuccess handler.web/src/components/plants/ScanPacketModal.tsx:143— Spacing validation usesparseFloat(spacing), whilequantityanddaysuse the stricterNumber()(lines 106, 149).parseFloataccepts trailing garbage (e.g."15abc"→15), so malformed spacing input can slip through where other fields would be rejected. Since the input istype="number"this is harder to trigger in practice, but the inconsistency weakens validation. Fix: replaceparseFloat(spacing)withNumber(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:
ScanPacketModalis 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_ICONinseedPacket.tsduplicate the values ofDEFAULT_COLOR/DEFAULT_ICONinPlantFormModal.tsx. They are intentionally decoupled (different semantic domains: manual create vs. packet scan), so this is defensible.onConfirmmirrorsSeedLotModalas 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.tsx—allandfilteredare correctly memoized withuseMemo;filterPlantsonly re-runs whenplants.data,query, orcategorychange. No new expensive computations are added to the render path.ScanPacketModal.tsx— The 12 state updates inonSuccessare batched by React 18 in an event handler, resulting in a single re-render.ReadFieldsis a separate module-level component and only re-renders whenproposalorunitchange. 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.useCreateFromPacketinvalidates bothplantsKeyandseedLotsKeytogether, which is the correct minimal invalidation.api.ts— TheFormDatabranch adds a trivialinstanceofcheck 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 clearsproposalto return to the capture phase, but it never clears theerrorstate. 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 reseterrorwheneverproposaltransitions tonull.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:
SeedPacketfields (species, variety, vendor, sku, lotCode, etc.) come from the vision model and are rendered inReadFieldsand the candidate list as React text children, which are auto-escaped — nodangerouslySetInnerHTMLanywhere in the diff (verified by grep across*.tsx). No injection vector.newPlantDefaults(seedPacket.ts:124) gatessuggestedCategorythroughisPlantCategoryagainstPLANT_CATEGORIES, falling back to'vegetable'. The UISelectonly offers the valid option set. An unknown model-supplied category cannot leak intoCreatePlant.POST /seed-lots/scanand/seed-lots/from-packetboth go throughmustActor(c)(verified ininternal/api/seed_packet.go:87,129), andCreateFromPacketenforces the ACL viavisiblePlantfor an existing-plant selection (internal/service/seed_packet.go:205) and the XOR invariant onplantId/newPlant(:189-192). The frontend adding a new path doesn't bypass the service checks.scanSeedPacketwraps the body inhttp.MaxBytesReader(30 MiB cap,seed_packet.go:48) and runsimagenorm.Normalizewhich 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.OLLAMA_CLOUD_API_KEYfrom the environment (never the DB, per CLAUDE.md); no user-controlled URL is fetched.sourceUrlis hardcoded to''client-side.postFormkeepscredentials: 'same-origin'and lets the browser set the multipart boundary (no attacker-controllable content-type).PlantIconcolor/icon injection: the candidates'color/iconoriginate from the actor's own existing plants (ListPlantsForActor), not from the untrusted vision output, andPlantIconis 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-153usesspacingCm < 1(cm) but the message says1 ${unitLabel}, which in imperial becomes "1 in" while the actual accepted minimum is ~0.39 in. Faithfully replicated fromPlantFormModal.tsx:87-88. ServerminPlantSpacingCM = 0.1.VERDICT: Minor issues
web/src/components/plants/ScanPacketModal.tsx:152-153— The spacing validation rejectsspacingCm < 1(a centimeter threshold) but the error message readsSpacing 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 types0.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 inPlantFormModal.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 againstinternal/service/plants.go:19(minPlantSpacingCM = 0.1).Everything else I checked is correct:
seedPacketSchema/packetProposalSchemadefaults matchvision.SeedPacketnullable fields; unknownsuggestedCategoryfalls back to'vegetable'viaisPlantCategory.lotDefaultsseed-count →seedsunit, elsepackets/qty 1 — matches the backend'sseedLotFieldsshape.candidates[0]?.plant.id ?? 'new') and the XOR ofplantId/newPlantmatchCreateFromPacket's "exactly one" rule.FormDatapath inapiFetchcorrectly omits the JSON content-type so the browser sets the multipart boundary.capabilities.visiondefault(false) means an older response hides the button rather than failing the parse (verifiedcapabilitiesSchema).onFilereset-then-read ordering is safe (filecaptured beforee.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 (
grepforvalidateLot/parseYear/parseCostCentsreturned 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. TheonConfirmqty / packed-for-year / cost validation blocks are character-for-character copies ofSeedLotModal.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: theQuantity must be a number, or left blank.string,costCents = Math.round(c * 100), and the1900/2200year range appear verbatim in both. There is no shared helper inlib/seedLots.ts(confirmed by grep —validateLot/parseYear/parseCostCentsdon't exist anywhere inweb/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: extractparseQuantity/parsePackedYear/parseCostCentspure functions intolib/seedLots.tsand 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'andPACKET_PLANT_ICON = '🌱'are the same values asDEFAULT_COLOR/DEFAULT_ICONinPlantFormModal.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: exportDEFAULT_COLOR/DEFAULT_ICONfrom a shared spot (e.g.lib/plants.tsnext toPlantInput) and import in both, dropping thePACKET_PLANT_*aliases.web/src/lib/seedPacket.ts:98-100—DEFAULT_SPACING_CM = 30claims sameness it can't enforce. The comment says "the same default the manual new-plant form uses", butPlantFormModal.tsx:61inlines the literal30(source?.spacingCm ?? 30) with no reference to this constant (verified by grep —DEFAULT_SPACING_CMappears only inseedPacket.ts, not inPlantFormModal). 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 oneDEFAULT_SPACING_CMconstant (inlib/plants.tsorlib/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. Theselection === 'new'branch re-implements spacing / days-to-maturity validation thatPlantFormModal.tsx:86-100already has (verified: the spacing< 1check with the sameSpacing 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 sharedvalidatePlantCoreFields(...)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/useCreateFromPacketare standard one-shot mutations; no caching concerns (intentionally uncached, each photo is fresh).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/unitOptionsare module-level constants, not rebuilt per render — good.proposal.candidatesis bounded server-side (maxPlantMatches = 10ininternal/service/plants.go:91), so the.mapin the review phase is bounded. Verified.setStatecalls inscan.mutate.onSuccessall fire inside one event-handler callback, so React 18 auto-batches them into a single re-render. Fine.useCreateFromPacket.onSuccessinvalidatesplantsKeyandseedLotsKey— both queries already run onPlantsPage, so the refetch is the expected, necessary cost of showing the new rows. No over- or under-invalidation.ReadFieldsbuilds a smallrowsarray per render (≤5 entries); trivial.useCapabilities()onPlantsPageadds one fetch, cached withstaleTime: 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/LotInputshapes to check thelotobject built inScanPacketModalagainst the actualSeedLotInputandPlantInput.Let me check
cmFromSpacingandspacingFromCmto verify NaN handling, and the backend seed-packet endpoints for what shapes they actually accept.parseFloat('')returnsNaN, andcmFromSpacing(NaN, ...)returnsroundCm(NaN*...). Let me checkroundCm:cmFromSpacing(NaN, unit)→roundCm(NaN)→NaN. Then!Number.isFinite(spacingCm) || spacingCm < 1catches NaN. Good.Now let me verify the backend endpoint to understand what happens with
newPlantfield ordering / extra fields, and whether thelotobject's fields match what's accepted. Let me also check whetherpurchasedAt: nulland other nulls are acceptable server-side.Now let me verify the API handler's binding shape for
from-packet— particularly whetherplantId(lowercase d) in the JSON is decoded correctly given the service usesPlantID.Now let me verify the
seedLotFieldsserver-side to check thelotobject built in the modal matches (esp.purchasedAt: nullvs optional string), andplantCreateRequestto checknewPlantfield shape (e.g.daysToMaturitynull handling,sourceUrletc.).Confirmed: server side
purchasedAt *string,costCents *int,germinationPct *float64,packedForYear *int,daysToMaturity *int. The modal sendspurchasedAt: 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)whenspacingis empty string. In theselection === 'new'path, validation catches NaN. Good.Let me also double-check:
onFileresetse.target.value = ''then readse.target.files?.[0]afterward. The reset happens before reading the file — does clearing.valuealso clear.files?The
e.target.value = ''after grabbingfileis a well-known safe pattern —fileholds the File reference, reset of.valuejust clears the input. That's fine.Now let me consider an important edge case in the error-handling lens: what if
scan.mutateis called while a previous scan is pending (rapid double-pick)? React Query mutations aren't queued by default;mutatewhile pending — defaultmutationKeynone — it'll just start a new mutation. Could cause two proposals landing and the secondonSuccessracing. 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
ReadFieldsrendering with a NaN edge case —spacingFromCm(p.spacingCm, unit)wherep.spacingCmcould 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:
spacingFromCm(p.spacingCm, unit)inReadFields(line 386 of modal): ifp.spacingCmis negative or 0, this renders a weird value. But it's read-only display — not a crash, minor.newPlantDefaultsspacing:p.packet.spacingCm ?? DEFAULT_SPACING_CM— if model returnsspacingCm: 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 < 1true → 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.Quantity:
Number('')is 0, butqty = quantity.trim() === '' ? 0 : Number(quantity).Number('abc')is NaN, caught.Number('1e308')etc. fine.Number('-5')→ -5, caught by< 0. Good. ButNumber('Infinity')→ Infinity,Number.isFinite(Infinity)false → error. Good.Cost:
Number(cost)thenMath.round(c * 100).ccould be huge.Number.MAX_SAFE_INTEGER * 100overflow not really an issue for typical currency. Fine.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') setsplantId = selectionbut does NOT validate that this plant id still exists/is visible. The backend handles that (visiblePlant→ ErrNotFound). But more importantly, whenselectionis a candidate plant id, andnewPlantis left undefined — good, exactly-one satisfied. Backend rejects both/neither. Fine.But there's a subtle issue: if
proposal.candidatesis 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 whenselection === 'new'). On submit with existing selection,newPlantundefined,plantIdset. Good.Now, one real concern:
cmFromSpacing(parseFloat(spacing), unit)only happens in theselection === 'new'branch. Good — existing-plant path doesn't need spacing. Fine.Let me check whether there's an issue with the
costfield being non-finite but theMath.round(c * 100)producing NaN that slips to backend. No —!Number.isFinite(c) || c < 0guards beforeMath.round. Good.Let me verify the
errorMessagefor the 503 VISION_DISABLED case actually surfaces a usable message (the backend writeserror.message). ThemessageFromreadsbody.error.message. Good. TheonErrorfallback is only used if not an ApiError. Fine.Let me check one more: the
scan.mutateonError— does React Query callonErrorfor a zod parse failure insidemutationFn? ThepacketProposalSchema.parsethrows a ZodError, which propagates out ofmutationFn→ React Query treats as mutation error →onErrorfires 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
PlantsPageintegration to verify thecapabilities.data?.visiongate handlescapabilitiesstill loading / error.The
capabilities.data?.visionoptional-chain handlesdatabeing undefined (loading/error) → falsy → button hidden. Good. Thevisiondefault(false) ensures older backends hide it. Good.Now let me check one potential real issue: when
capabilitiesquery errors (network),datais undefined, button hidden — fine, no crash. When the query is stale and shows oldvision:truethen backend disabled vision — the modal's scan would 503, handled viaonError. Fine.Let me check the
Modalbusyprop behavior — doesbusyblock the close (X) button? If a scan is pending and user hits Escape or backdrop, does it cancel? Let me check Modal.Modalbusyblocks Escape and backdrop close. Good — so during a scan the modal stays. The Cancel buttons aredisabled={busy}. Good. The "Rescan" button (setsproposal(null)) isdisabled={busy}— good, can't rescan mid-scan.But wait — there's a subtle edge:
busy = scan.isPending || create.isPending. During review phase,scan.isPendingis false (scan done) andcreate.isPendingis false until submit. Sobusyis false in review — fine. During capture,scan.isPendingtrue → 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.mutatebeing called when a previouscreateis pending could interleave. The capture phase only shows when!proposal, andproposalis set on scan success. Once in review, capture input not shown. So no double-scan. Fine.Let me check the
costedge:cost.trim()thenNumber(cost)(notcost.trim()). Minor inconsistency butNumber(' 4.99 ')= 4.99, fine. Actually they checkcost.trim()for emptiness but passcost(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.packedForYearisnumber | nullfromlotDefaults. If it's a number,String(number). Fine. ButlotDefaultsreturnspackedForYear: p.packedForYearwherep.packedForYearisnumber | null. Good.Let me verify the
spacingdefault display:setSpacing(String(spacingFromCm(plant.spacingCm, unit))).plant.spacingCmfromnewPlantDefaultsisp.packet.spacingCm ?? DEFAULT_SPACING_CM→ always a number.spacingFromCmreturns a number.String(number). If spacingCm is e.g. 30 → "30" metric, or imperial 30cm → 11.8 → "11.8". Fine. IfspacingCmwere NaN somehow,String(NaN)= "NaN", and on submitparseFloat("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
lotobject sent on confirm is missing nothing required, butunitvalidation. BackendseedLotFields.Unithasbinding:"required". The modal always sendslotUnit(default 'packets'), so always present. Good.Let me check whether there's an issue with
germinationPct: null— backendGerminationPct *float64accepts null. TheSeedLotInputTS type requiresgerminationPct: number | null. Modal sendsnull. Good.Let me verify the
SeedLotFieldstype matches what modal builds (TypeScript-level).SeedLotFields = Omit<SeedLotInput, 'plantId'>.SeedLotInputhas: 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)whenquantityis something like" "(whitespace only) —quantity.trim() === '' ? 0 : Number(quantity)." ".trim()= "" → 0. Good. ButNumber(" 5 ")= 5. Good.Number("5abc")= NaN → caught. Good.Let me check:
Number(days)for daysToMaturity —days.trim()thenNumber(days).Number("90")= 90.Number("90.5")= 90.5,Number.isInteger(90.5)false → error. Good.Number("")would be 0 but guarded bydays.trim(). Good.I think the error handling is quite solid. Let me do a final check on one thing: the
onConfirmisasyncand usesawait create.mutateAsync(...). If the component unmounts mid-await (user closes modal? No — busy blocks close). ActuallyonClose()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. Thecatchsets error — component still mounted. Fine.Let me check one last potential:
create.mutateAsyncthrowing anApiErrorwithisConflict(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,errorMessagesurfaces the message. No special conflict-rebase handling like other forms. That's an inconsistency vs SeedLotModal which callsconflictSeedLot. 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
visiondefault(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 througherrorMessagewith sensible fallbacks.The only minor items I found:
spacingFromCm(p.spacingCm, unit)inReadFieldswill render a nonsensical value if the model returns a negative/zerospacingCm, 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:
onConfirm(ScanPacketModal.tsx:106-119,:127-135,:138-144):quantity,packedForYear,cost,spacing,daysall trim-check emptiness, thenNumber.isFinite/Number.isInteger/range checks before use.cmFromSpacing(parseFloat(spacing), unit)producing NaN is caught by!Number.isFinite(spacingCm) || spacingCm < 1(:128). ConfirmedroundCm/cmFromSpacingpropagate NaN (units.ts:29-31, 191-193), so the guard is load-bearing and correct.lotobject (:121-134) andnewPlant(:146-154) matchseedLotFields(seed_lots.go:24-36) andplantCreateRequest(plants.go:16-26) — nullable Go pointers accept thenulls the client sends (purchasedAt,costCents,germinationPct,packedForYear,daysToMaturity). Verified the TSSeedLotFields = Omit<SeedLotInput,'plantId'>and the modal builds exactly those keys.visiondefault(false) (agent.ts) +capabilities.data?.visiongate (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 = scan.isPending || create.isPendingflows toModal busywhich blocks Escape/backdrop (Modal.tsx:47,94), and the Cancel/Rescan buttons aredisabled={busy}(:319,:323). No setState-after-unmount in the asynconConfirmsuccess path (onClose only after await).VISION_DISABLED/ unreadable / too-large all returnerror.messageviamessageFrom(api.ts:80-88), surfaced througherrorMessage(err, fallback)(:95-96). A non-ApiError(e.g. a zod parse failure insidemutationFn) falls back to the generic message — degrades gracefully, no crash.selection==='new'→ newPlant, else → plantId), so neither "both" nor "neither" can occur.e.target.value = ''after grabbingfile(: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):
ReadFieldsrendersspacingFromCm(p.spacingCm, unit)for any non-nullspacingCm; 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:
FormData→api.postForm→ backendhttp.MaxBytesReader→imagenorm.Normalize(JPEG re-encode). No filename-based path injection; content is normalized server-side before reaching the vision model.name,vendor,sku,lotCode,quantity,cost,spacing,days) are validated (trim, numeric checks, range bounds) before submission.parseFloat/Numberon empty strings produceNaN, which the existing!Number.isFinite(...)guard rejects.plant.idselection is an integer validated byplantSchemaon arrival; no path-traversal or injection vector.agentandvisiontofalse— a secure-by-default improvement that hides features on partial/older responses instead of exposing them or crashing the parse.dangerouslySetInnerHTMLis introduced.ApiError) are user-facing static strings in the Go handlers; no stack-trace or internal-path leakage.OLLAMA_CLOUD_API_KEYstays env-only per DESIGN.md, and the UI only sees a booleanvisioncapability.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— TheonConfirmhandler mirrorsSeedLotModalexactly: 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.isFinitecorrectly rejects non-numeric and out-of-range inputs.Unit conversions —
spacingFromCm/cmFromSpacinground-trip correctly for both metric and imperial. TheroundCmdust-guard (1e-6) is applied consistently.Backend contract alignment —
FromPacketBodysends exactly one ofplantId(number) ornewPlant(PlantInput), matching the Go structfromPacketRequestwhich demands exactly one viaPlantID *int64XORNewPlant *plantCreateRequest. The lot shape (SeedLotFields = Omit<SeedLotInput, 'plantId'>) matchesseedLotFields.toInput()on the server.Schema defaults —
seedPacketSchemadefaults every field safely.newPlantDefaultsfalls back to'vegetable'for unknown categories (defensive against model hallucination) and30cm for missing spacing, matching the manual new-plant form.lotDefaultscorrectly interprets a positiveseedCountas seeds and defaults to 1 packet otherwise.FormData upload —
api.postFormpassesFormDatathrough unchanged with no JSON content-type header, letting the browser set the multipart boundary. Theinstanceof FormDataguard inapiFetchis safe and backward-compatible.Capabilities schema — Adding
vision: z.boolean().default(false)alongsideagentmeans older/partial responses hide the feature rather than failing parse. This is the correct defensive choice.Query invalidation —
useCreateFromPacketinvalidatesplantsKeyandseedLotsKeyon 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 fromSeedLotModal.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—categoryOptionsis identical to the one inPlantFormModal.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—unitOptionsduplicates the inline expression inSeedLotModal.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:
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.useCreateFromPacketbroadly invalidatesplantsKeyandseedLotsKey. 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.matchPlantson 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:
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 firesonChange.Number.isFinite,Number.isInteger, and range checks (< 1,< 0,< 1900,> 2200) are applied to every parsed field.parseFloat('')→NaNis caught by!Number.isFinite.Number('abc')→NaNis likewise caught.cmFromSpacing(parseFloat(spacing), unit)is validated with!Number.isFinite(spacingCm) || spacingCm < 1, which catchesNaN,Infinity, negative values, and zero/negative-zero.costCents = Math.round(c * 100)could theoretically exceedNumber.MAX_SAFE_INTEGERfor absurd inputs, but this is a server-validated field and the client guards (Number.isFinite,c < 0) are appropriate for the UI layer.busy = scan.isPending || create.isPendingis piped into theModal’sbusyprop, 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.capabilitiesSchemadefaults bothagentandvisiontofalse, so an older/partial server response hides the feature rather than failing parse.seedPacketSchemadefaults every field, andnewPlantDefaultsfalls back unknown categories to'vegetable'and missing spacing to30cm.instanceof FormDatacheck inapiFetchcorrectly omits the JSON content-type so the browser generates the multipart boundary.postFormis 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.