From 4597978034d8a16cb19d1e7801ea3fbd89dba8fb Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 02:01:58 -0400 Subject: [PATCH] Address Gadfly review on the seed shelf UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The correctness finding is a real bug and the interesting one: arming a plant from the Seed Tray never attributed a seed lot, not even in the unambiguous single-lot case. The tray is the QUICKEST way to place a plant, so the fastest path was the one that silently lost which packet the seed came from — and it would have looked fine, because attribution is invisible until you go looking at a remaining count that's wrong. armPlant now does the same single-lot auto-attribution the picker does. Several lots stays unattributed rather than guessing; the picker is where that choice belongs. A single exhausted lot was also indistinguishable from having no lot, because the "available" filter dropped it before the count was taken. That silently dropped attribution with no signal. attributableLots now offers exhausted lots too, just not first: the count records what was written down, not a fact about the packet, so refusing to attribute a planting because the number says zero makes a wrong number harder to correct rather than easier. It's also now one rule in one place instead of the same filter written twice. The seed lot form had no 409 handling, alone among the version-guarded forms here. A concurrent edit made you retype everything; it now rebases onto the current row and says so, like the garden and plant forms do. Deduplication: the outbound-link anchor was duplicated verbatim between the plant card and the lot rows, including the security-relevant rel attribute — now one SourceLink that re-checks the URL and renders nothing when it isn't safe, so callers can't forget to guard. remainingLabel had its own copy of the unit-agreement rule that summarizeLots already owns; it now calls it. formatQuantity moved out of a component file into lib/seedLots.ts with the other formatters. STATE_CLASS.over and .low were identical. "Over-planted" is a discrepancy to look at, not a shortage to act on, so it now reads as its own warning. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- .../components/plants/DeleteSeedLotModal.tsx | 3 +- web/src/components/plants/PlantCard.tsx | 22 ++++--------- web/src/components/plants/SeedLotList.tsx | 27 ++++------------ web/src/components/plants/SeedLotModal.tsx | 28 ++++++++++++++++- web/src/components/plants/SourceLink.tsx | 27 ++++++++++++++++ web/src/editor/PlantPicker.tsx | 31 ++++++++++++------- web/src/lib/seedLots.test.ts | 28 +++++++++++++++++ web/src/lib/seedLots.ts | 29 ++++++++++++++++- web/src/pages/GardenEditorPage.tsx | 13 +++++++- 9 files changed, 155 insertions(+), 53 deletions(-) create mode 100644 web/src/components/plants/SourceLink.tsx diff --git a/web/src/components/plants/DeleteSeedLotModal.tsx b/web/src/components/plants/DeleteSeedLotModal.tsx index f87c25b..5d7dc00 100644 --- a/web/src/components/plants/DeleteSeedLotModal.tsx +++ b/web/src/components/plants/DeleteSeedLotModal.tsx @@ -3,8 +3,7 @@ import { Alert } from '@/components/ui/Alert' import { Button } from '@/components/ui/Button' import { Modal } from '@/components/ui/Modal' import { errorMessage } from '@/lib/api' -import { useDeleteSeedLot, type SeedLot } from '@/lib/seedLots' -import { formatQuantity } from './SeedLotList' +import { formatQuantity, useDeleteSeedLot, type SeedLot } from '@/lib/seedLots' /** * Retire a lot. Worth confirming because it's the one place cost and germination diff --git a/web/src/components/plants/PlantCard.tsx b/web/src/components/plants/PlantCard.tsx index 6906f67..2a8d771 100644 --- a/web/src/components/plants/PlantCard.tsx +++ b/web/src/components/plants/PlantCard.tsx @@ -1,9 +1,10 @@ import { useState } from 'react' import { PlantIcon } from './PlantIcon' -import { LotStateChip, SeedLotList, formatQuantity } from './SeedLotList' +import { LotStateChip, SeedLotList } from './SeedLotList' +import { SourceLink } from './SourceLink' import { cardActionClass, cardDangerClass } from '@/components/ui/cardActions' import { CATEGORY_LABELS, isBuiltin, type Plant } from '@/lib/plants' -import { safeExternalUrl, summarizeLots, type SeedLot } from '@/lib/seedLots' +import { formatQuantity, summarizeLots, type SeedLot } from '@/lib/seedLots' import { formatSpacing, type UnitPref } from '@/lib/units' /** @@ -37,7 +38,7 @@ export function PlantCard({ const builtin = isBuiltin(plant) const [showLots, setShowLots] = useState(false) const summary = summarizeLots(lots) - const sourceUrl = safeExternalUrl(plant.sourceUrl) + return (
@@ -54,21 +55,10 @@ export function PlantCard({

{CATEGORY_LABELS[plant.category]} · {formatSpacing(plant.spacingCm, unit)} spacing

- {(plant.vendor || sourceUrl) && ( + {(plant.vendor || plant.sourceUrl) && (

{plant.vendor && {plant.vendor}} - {sourceUrl && ( - - source ↗ - - )} +

)} {plant.notes &&

{plant.notes}

} diff --git a/web/src/components/plants/SeedLotList.tsx b/web/src/components/plants/SeedLotList.tsx index 9f8dd8a..afff2c0 100644 --- a/web/src/components/plants/SeedLotList.tsx +++ b/web/src/components/plants/SeedLotList.tsx @@ -1,10 +1,11 @@ import { Button } from '@/components/ui/Button' import { cn } from '@/lib/cn' +import { SourceLink } from './SourceLink' import { formatCost, + formatQuantity, formatUnitCost, lotState, - safeExternalUrl, type LotState, type SeedLot, } from '@/lib/seedLots' @@ -20,7 +21,9 @@ const STATE_LABEL: Record = { // Encoded in colour AND words, because this is the thing you skim down a list of // twenty packets deciding what to order — a bare number doesn't survive that. const STATE_CLASS: Record = { - over: 'bg-amber-500/20 text-amber-800 dark:text-amber-300', + // "over" is a discrepancy to look at rather than a shortage to act on, so it + // reads as a distinct warning rather than sharing "low"'s styling. + over: 'bg-orange-500/25 text-orange-900 dark:text-orange-200', empty: 'bg-red-500/15 text-red-800 dark:text-red-300', low: 'bg-amber-500/20 text-amber-800 dark:text-amber-300', ok: 'bg-accent/20 text-accent-strong', @@ -110,7 +113,6 @@ function LotRow({ onEdit: () => void onDelete: () => void }) { - const url = safeExternalUrl(lot.sourceUrl) const cost = formatCost(lot.costCents) const unitCost = formatUnitCost(lot) @@ -130,18 +132,7 @@ function LotRow({ {lot.purchasedAt && bought {lot.purchasedAt}} {lot.germinationPct != null && {lot.germinationPct}% germ.} {cost && {unitCost ? `${cost} (${unitCost})` : cost}} - {url && ( - - source ↗ - - )} +

@@ -168,9 +159,3 @@ function LotRow({
) } - -/** Quantities are stored as REAL because grams and ounces are fractional, but a - * whole number of seeds shouldn't render as "100.0". */ -export function formatQuantity(n: number): string { - return Number.isInteger(n) ? String(n) : n.toFixed(1) -} diff --git a/web/src/components/plants/SeedLotModal.tsx b/web/src/components/plants/SeedLotModal.tsx index d0b0552..84d2e09 100644 --- a/web/src/components/plants/SeedLotModal.tsx +++ b/web/src/components/plants/SeedLotModal.tsx @@ -7,6 +7,7 @@ import { TextArea } from '@/components/ui/TextArea' import { TextField } from '@/components/ui/TextField' import { errorMessage } from '@/lib/api' import { + conflictSeedLot, LOT_UNITS, safeExternalUrl, useCreateSeedLot, @@ -49,11 +50,14 @@ export function SeedLotModal({ const [sku, setSku] = useState(lot?.sku ?? '') const [lotCode, setLotCode] = useState(lot?.lotCode ?? '') const [notes, setNotes] = useState(lot?.notes ?? '') + const [version, setVersion] = useState(lot?.version ?? 0) + const [conflict, setConflict] = useState(null) const [error, setError] = useState(null) async function onSubmit(e: FormEvent) { e.preventDefault() setError(null) + setConflict(null) const qty = quantity.trim() === '' ? 0 : Number(quantity) if (!Number.isFinite(qty) || qty < 0) { @@ -109,12 +113,32 @@ export function SeedLotModal({ try { if (isEdit) { - await update.mutateAsync({ id: lot.id, version: lot.version, ...input }) + await update.mutateAsync({ id: lot.id, version, ...input }) } else { await create.mutateAsync(input) } onClose() } catch (err) { + const current = conflictSeedLot(err) + if (current) { + // Someone edited this lot elsewhere. Rebase onto the fresh row so a + // re-save applies, rather than making them retype everything — the same + // contract every other version-guarded form here honours. + setVersion(current.version) + setVendor(current.vendor) + setSourceUrl(current.sourceUrl) + setQuantity(String(current.quantity)) + setUnit(current.unit) + setPurchasedAt(current.purchasedAt ?? '') + setPackedForYear(current.packedForYear != null ? String(current.packedForYear) : '') + setCost(current.costCents != null ? (current.costCents / 100).toFixed(2) : '') + setGermination(current.germinationPct != null ? String(current.germinationPct) : '') + setSku(current.sku) + setLotCode(current.lotCode) + setNotes(current.notes) + setConflict('This lot changed elsewhere. The latest values are shown — review and save again.') + return + } setError(errorMessage(err, isEdit ? 'Could not save the lot.' : 'Could not record the lot.')) } } @@ -122,6 +146,8 @@ export function SeedLotModal({ return (
+ {conflict && {conflict}} +
+ {label} ↗ + + ) +} diff --git a/web/src/editor/PlantPicker.tsx b/web/src/editor/PlantPicker.tsx index 10d2cd5..8f2d5d7 100644 --- a/web/src/editor/PlantPicker.tsx +++ b/web/src/editor/PlantPicker.tsx @@ -4,8 +4,16 @@ import { fieldControlClass } from '@/components/ui/field' import { CategoryChips } from '@/components/plants/CategoryChips' import { PlantIcon } from '@/components/plants/PlantIcon' import { CATEGORY_LABELS, filterPlants, usePlants, type CategoryFilter, type Plant } from '@/lib/plants' -import { lotsByPlant, lotState, useSeedLots, type SeedLot } from '@/lib/seedLots' -import { LotStateChip, formatQuantity } from '@/components/plants/SeedLotList' +import { + attributableLots, + formatQuantity, + lotsByPlant, + lotState, + summarizeLots, + useSeedLots, + type SeedLot, +} from '@/lib/seedLots' +import { LotStateChip } from '@/components/plants/SeedLotList' import { formatSpacing, type UnitPref } from '@/lib/units' const RECENT_KEY = 'pansy:recent-plants' @@ -85,12 +93,15 @@ export function PlantPicker({ }, [all, recent, query]) function choose(p: Plant) { - const lots = (lotsFor.get(p.id) ?? []).filter((l) => l.remaining > 0) + const lots = attributableLots(lotsFor.get(p.id) ?? []) if (lots.length > 1) { setChoosingLotFor(p) return } setRecent(recordRecent(p.id)) + // A single lot attributes even when its count says empty. The count records + // what was written down, not a fact about the packet — dropping attribution + // there would make a wrong number harder to correct rather than easier. onSelect(p, lots[0]) } @@ -121,12 +132,12 @@ export function PlantPicker({ // What's left, shown where you're deciding what to plant. Silent when there // are no lots — most plants won't have any, and "0 left" on all of them would - // be noise that trains you to ignore the number. + // be noise that trains you to ignore the number. summarizeLots owns the + // unit-agreement rule; a second copy of it here would drift. function remainingLabel(lots: SeedLot[]): string { if (lots.length === 0) return '' - const unitName = lots.every((l) => l.unit === lots[0].unit) ? ` ${lots[0].unit}` : '' - const total = lots.reduce((sum, l) => sum + l.remaining, 0) - return ` · ${formatQuantity(total)}${unitName} left` + const { remaining, unit } = summarizeLots(lots) + return ` · ${formatQuantity(remaining)}${unit ? ` ${unit}` : ''} left` } return ( @@ -169,9 +180,7 @@ export function PlantPicker({

Which lot of {choosingLotFor.name}?

- {(lotsFor.get(choosingLotFor.id) ?? []) - .filter((l) => l.remaining > 0) - .map((lot) => ( + {attributableLots(lotsFor.get(choosingLotFor.id) ?? []).map((lot) => ( - ))} + ))}