Address Gadfly review on #13: real bugs + dedupe page/picker
Build image / build-and-push (push) Successful in 8s
Build image / build-and-push (push) Successful in 8s
Correctness: - PlantFormModal: validate days-to-maturity with Number() so "1.5" is rejected rather than silently truncated to 1 (parseInt). Accurate "at least 1 <unit>" spacing message; spacing input min now matches validation. Drop the icon maxLength that could split a multi-codepoint emoji (server caps at 32 bytes). - PlantPicker: namespace row keys (recent- / all-) so a plant shown in both the Recent and All sections no longer collides on a duplicate React key. Dedupe (the review's dominant, multi-model theme) — also sets up #15's reuse: - lib/plants.ts: shared CategoryFilter type + filterPlants() helper. - components/plants/CategoryChips.tsx and PlantIcon.tsx: extracted the chip row and the color-mix icon tile, now used by both the /plants page and PlantPicker (and PlantCard for the tile). Skipped (cosmetic/cross-file, low agreement): conflictPlant→generic helper, PlantCard vs GardenCard button-class dup, form component length, and localStorage user-scoping (recent custom-plant ids don't resolve cross-user anyway). tsc --noEmit clean; 20/20 vitest; production build green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
This commit is contained in:
@@ -0,0 +1,37 @@
|
|||||||
|
import { cn } from '@/lib/cn'
|
||||||
|
import { CATEGORY_LABELS, PLANT_CATEGORIES, type CategoryFilter } from '@/lib/plants'
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Horizontal, scrollable "All + each category" chip row. Shared by the /plants
|
||||||
|
* page and the PlantPicker so both filter the catalog identically.
|
||||||
|
*/
|
||||||
|
export function CategoryChips({
|
||||||
|
value,
|
||||||
|
onChange,
|
||||||
|
size = 'md',
|
||||||
|
}: {
|
||||||
|
value: CategoryFilter
|
||||||
|
onChange: (c: CategoryFilter) => void
|
||||||
|
size?: 'sm' | 'md'
|
||||||
|
}) {
|
||||||
|
const chip = (v: CategoryFilter, label: string) => (
|
||||||
|
<button
|
||||||
|
key={v}
|
||||||
|
type="button"
|
||||||
|
onClick={() => onChange(v)}
|
||||||
|
className={cn(
|
||||||
|
'shrink-0 rounded-full px-3 font-medium transition-colors',
|
||||||
|
size === 'sm' ? 'py-1 text-xs' : 'py-1 text-sm',
|
||||||
|
value === v ? 'bg-accent text-accent-contrast' : 'bg-border/50 text-muted hover:text-fg',
|
||||||
|
)}
|
||||||
|
>
|
||||||
|
{label}
|
||||||
|
</button>
|
||||||
|
)
|
||||||
|
return (
|
||||||
|
<div className="flex gap-1.5 overflow-x-auto">
|
||||||
|
{chip('all', 'All')}
|
||||||
|
{PLANT_CATEGORIES.map((c) => chip(c, CATEGORY_LABELS[c]))}
|
||||||
|
</div>
|
||||||
|
)
|
||||||
|
}
|
||||||
@@ -1,3 +1,4 @@
|
|||||||
|
import { PlantIcon } from './PlantIcon'
|
||||||
import { CATEGORY_LABELS, isBuiltin, type Plant } from '@/lib/plants'
|
import { CATEGORY_LABELS, isBuiltin, type Plant } from '@/lib/plants'
|
||||||
import { formatSpacing, type UnitPref } from '@/lib/units'
|
import { formatSpacing, type UnitPref } from '@/lib/units'
|
||||||
|
|
||||||
@@ -30,13 +31,7 @@ export function PlantCard({
|
|||||||
return (
|
return (
|
||||||
<div className="flex flex-col rounded-xl border border-border bg-surface">
|
<div className="flex flex-col rounded-xl border border-border bg-surface">
|
||||||
<div className="flex items-start gap-3 p-4">
|
<div className="flex items-start gap-3 p-4">
|
||||||
<span
|
<PlantIcon color={plant.color} icon={plant.icon} className="h-11 w-11 rounded-lg text-2xl" />
|
||||||
className="grid h-11 w-11 shrink-0 place-items-center rounded-lg text-2xl"
|
|
||||||
style={{ backgroundColor: `color-mix(in srgb, ${plant.color} 18%, transparent)` }}
|
|
||||||
aria-hidden
|
|
||||||
>
|
|
||||||
{plant.icon}
|
|
||||||
</span>
|
|
||||||
<div className="min-w-0 flex-1">
|
<div className="min-w-0 flex-1">
|
||||||
<div className="flex items-center gap-2">
|
<div className="flex items-center gap-2">
|
||||||
<h3 className="truncate font-semibold text-fg">{plant.name}</h3>
|
<h3 className="truncate font-semibold text-fg">{plant.name}</h3>
|
||||||
|
|||||||
@@ -65,6 +65,7 @@ export function PlantFormModal({
|
|||||||
const [version, setVersion] = useState(plant?.version ?? 0)
|
const [version, setVersion] = useState(plant?.version ?? 0)
|
||||||
const [conflict, setConflict] = useState<string | null>(null)
|
const [conflict, setConflict] = useState<string | null>(null)
|
||||||
const [formError, setFormError] = useState<string | null>(null)
|
const [formError, setFormError] = useState<string | null>(null)
|
||||||
|
const unitLabel = spacingUnitLabel(unit)
|
||||||
|
|
||||||
async function onSubmit(e: FormEvent) {
|
async function onSubmit(e: FormEvent) {
|
||||||
e.preventDefault()
|
e.preventDefault()
|
||||||
@@ -81,12 +82,13 @@ export function PlantFormModal({
|
|||||||
}
|
}
|
||||||
const spacingCm = cmFromSpacing(parseFloat(spacing), unit)
|
const spacingCm = cmFromSpacing(parseFloat(spacing), unit)
|
||||||
if (!Number.isFinite(spacingCm) || spacingCm < 1) {
|
if (!Number.isFinite(spacingCm) || spacingCm < 1) {
|
||||||
setFormError('Spacing must be a positive number.')
|
setFormError(`Spacing must be at least 1 ${unitLabel}.`)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
let daysToMaturity: number | null = null
|
let daysToMaturity: number | null = null
|
||||||
if (days.trim()) {
|
if (days.trim()) {
|
||||||
const d = parseInt(days, 10)
|
// Number() (not parseInt) so "1.5" is rejected, not silently truncated to 1.
|
||||||
|
const d = Number(days)
|
||||||
if (!Number.isInteger(d) || d < 1) {
|
if (!Number.isInteger(d) || d < 1) {
|
||||||
setFormError('Days to maturity must be a whole number of days, or left blank.')
|
setFormError('Days to maturity must be a whole number of days, or left blank.')
|
||||||
return
|
return
|
||||||
@@ -131,8 +133,6 @@ export function PlantFormModal({
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
const unitLabel = spacingUnitLabel(unit)
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<Modal title={isEdit ? 'Edit plant' : 'New plant'} onClose={onClose} busy={pending}>
|
<Modal title={isEdit ? 'Edit plant' : 'New plant'} onClose={onClose} busy={pending}>
|
||||||
<form onSubmit={onSubmit} className="flex flex-col gap-3">
|
<form onSubmit={onSubmit} className="flex flex-col gap-3">
|
||||||
@@ -154,7 +154,7 @@ export function PlantFormModal({
|
|||||||
type="number"
|
type="number"
|
||||||
inputMode="decimal"
|
inputMode="decimal"
|
||||||
step="any"
|
step="any"
|
||||||
min="0"
|
min="1"
|
||||||
required
|
required
|
||||||
value={spacing}
|
value={spacing}
|
||||||
onChange={(e) => setSpacing(e.target.value)}
|
onChange={(e) => setSpacing(e.target.value)}
|
||||||
@@ -178,7 +178,6 @@ export function PlantFormModal({
|
|||||||
label="Icon (emoji)"
|
label="Icon (emoji)"
|
||||||
name="icon"
|
name="icon"
|
||||||
value={icon}
|
value={icon}
|
||||||
maxLength={8}
|
|
||||||
onChange={(e) => setIcon(e.target.value)}
|
onChange={(e) => setIcon(e.target.value)}
|
||||||
hint="A single emoji, e.g. 🍅"
|
hint="A single emoji, e.g. 🍅"
|
||||||
/>
|
/>
|
||||||
|
|||||||
@@ -0,0 +1,18 @@
|
|||||||
|
import { cn } from '@/lib/cn'
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A plant's emoji on a tile tinted with its own color (via color-mix, so any
|
||||||
|
* valid CSS color works). Shared by PlantCard and the PlantPicker rows. Size and
|
||||||
|
* shape come from `className`.
|
||||||
|
*/
|
||||||
|
export function PlantIcon({ color, icon, className }: { color: string; icon: string; className?: string }) {
|
||||||
|
return (
|
||||||
|
<span
|
||||||
|
className={cn('grid shrink-0 place-items-center', className)}
|
||||||
|
style={{ backgroundColor: `color-mix(in srgb, ${color} 18%, transparent)` }}
|
||||||
|
aria-hidden
|
||||||
|
>
|
||||||
|
{icon}
|
||||||
|
</span>
|
||||||
|
)
|
||||||
|
}
|
||||||
@@ -1,7 +1,9 @@
|
|||||||
import { useEffect, useMemo, useRef, useState } from 'react'
|
import { useEffect, useMemo, useRef, useState } from 'react'
|
||||||
import { cn } from '@/lib/cn'
|
import { cn } from '@/lib/cn'
|
||||||
import { fieldControlClass } from '@/components/ui/field'
|
import { fieldControlClass } from '@/components/ui/field'
|
||||||
import { CATEGORY_LABELS, PLANT_CATEGORIES, usePlants, type Plant, type PlantCategory } from '@/lib/plants'
|
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 { formatSpacing, type UnitPref } from '@/lib/units'
|
import { formatSpacing, type UnitPref } from '@/lib/units'
|
||||||
|
|
||||||
const RECENT_KEY = 'pansy:recent-plants'
|
const RECENT_KEY = 'pansy:recent-plants'
|
||||||
@@ -28,8 +30,6 @@ function recordRecent(id: number): number[] {
|
|||||||
return next
|
return next
|
||||||
}
|
}
|
||||||
|
|
||||||
type CategoryFilter = PlantCategory | 'all'
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Reusable plant chooser: a search-first list with a recently-used shortcut and
|
* Reusable plant chooser: a search-first list with a recently-used shortcut and
|
||||||
* category filter, rendered as a bottom sheet on mobile / centered panel on
|
* category filter, rendered as a bottom sheet on mobile / centered panel on
|
||||||
@@ -64,12 +64,7 @@ export function PlantPicker({
|
|||||||
|
|
||||||
const all = useMemo(() => plants.data ?? [], [plants.data])
|
const all = useMemo(() => plants.data ?? [], [plants.data])
|
||||||
|
|
||||||
const filtered = useMemo(() => {
|
const filtered = useMemo(() => filterPlants(all, query, category), [all, query, category])
|
||||||
const q = query.trim().toLowerCase()
|
|
||||||
return all.filter(
|
|
||||||
(p) => (category === 'all' || p.category === category) && (q === '' || p.name.toLowerCase().includes(q)),
|
|
||||||
)
|
|
||||||
}, [all, query, category])
|
|
||||||
|
|
||||||
// Recents show only when not actively searching, and only rows still present.
|
// Recents show only when not actively searching, and only rows still present.
|
||||||
const recentPlants = useMemo(() => {
|
const recentPlants = useMemo(() => {
|
||||||
@@ -83,20 +78,16 @@ export function PlantPicker({
|
|||||||
onSelect(p)
|
onSelect(p)
|
||||||
}
|
}
|
||||||
|
|
||||||
const row = (p: Plant) => (
|
// A plant option row. keyPrefix namespaces the key so a plant appearing in
|
||||||
|
// both the Recent and All sections doesn't collide on a duplicate React key.
|
||||||
|
const row = (p: Plant, keyPrefix: string) => (
|
||||||
<button
|
<button
|
||||||
key={p.id}
|
key={`${keyPrefix}-${p.id}`}
|
||||||
type="button"
|
type="button"
|
||||||
onClick={() => choose(p)}
|
onClick={() => choose(p)}
|
||||||
className="flex w-full items-center gap-3 rounded-lg px-3 py-2.5 text-left outline-none transition-colors hover:bg-border/50 focus-visible:bg-border/50"
|
className="flex w-full items-center gap-3 rounded-lg px-3 py-2.5 text-left outline-none transition-colors hover:bg-border/50 focus-visible:bg-border/50"
|
||||||
>
|
>
|
||||||
<span
|
<PlantIcon color={p.color} icon={p.icon} className="h-9 w-9 rounded-md text-xl" />
|
||||||
className="grid h-9 w-9 shrink-0 place-items-center rounded-md text-xl"
|
|
||||||
style={{ backgroundColor: `color-mix(in srgb, ${p.color} 18%, transparent)` }}
|
|
||||||
aria-hidden
|
|
||||||
>
|
|
||||||
{p.icon}
|
|
||||||
</span>
|
|
||||||
<span className="min-w-0 flex-1">
|
<span className="min-w-0 flex-1">
|
||||||
<span className="block truncate font-medium text-fg">{p.name}</span>
|
<span className="block truncate font-medium text-fg">{p.name}</span>
|
||||||
<span className="block text-xs text-muted">
|
<span className="block text-xs text-muted">
|
||||||
@@ -106,20 +97,6 @@ export function PlantPicker({
|
|||||||
</button>
|
</button>
|
||||||
)
|
)
|
||||||
|
|
||||||
const chip = (value: CategoryFilter, label: string) => (
|
|
||||||
<button
|
|
||||||
key={value}
|
|
||||||
type="button"
|
|
||||||
onClick={() => setCategory(value)}
|
|
||||||
className={cn(
|
|
||||||
'shrink-0 rounded-full px-3 py-1 text-xs font-medium transition-colors',
|
|
||||||
category === value ? 'bg-accent text-accent-contrast' : 'bg-border/50 text-muted hover:text-fg',
|
|
||||||
)}
|
|
||||||
>
|
|
||||||
{label}
|
|
||||||
</button>
|
|
||||||
)
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div
|
<div
|
||||||
className="fixed inset-0 z-50 flex items-end justify-center bg-black/40 sm:items-center sm:p-4"
|
className="fixed inset-0 z-50 flex items-end justify-center bg-black/40 sm:items-center sm:p-4"
|
||||||
@@ -151,9 +128,8 @@ export function PlantPicker({
|
|||||||
</button>
|
</button>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
<div className="flex gap-1.5 overflow-x-auto border-b border-border px-3 py-2">
|
<div className="border-b border-border px-3 py-2">
|
||||||
{chip('all', 'All')}
|
<CategoryChips value={category} onChange={setCategory} size="sm" />
|
||||||
{PLANT_CATEGORIES.map((c) => chip(c, CATEGORY_LABELS[c]))}
|
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
<div className="min-h-0 flex-1 overflow-y-auto p-2">
|
<div className="min-h-0 flex-1 overflow-y-auto p-2">
|
||||||
@@ -163,12 +139,12 @@ export function PlantPicker({
|
|||||||
{recentPlants.length > 0 && (
|
{recentPlants.length > 0 && (
|
||||||
<>
|
<>
|
||||||
<p className="px-3 pb-1 pt-2 text-xs font-semibold uppercase tracking-wide text-muted">Recent</p>
|
<p className="px-3 pb-1 pt-2 text-xs font-semibold uppercase tracking-wide text-muted">Recent</p>
|
||||||
{recentPlants.map(row)}
|
{recentPlants.map((p) => row(p, 'recent'))}
|
||||||
<p className="px-3 pb-1 pt-3 text-xs font-semibold uppercase tracking-wide text-muted">All plants</p>
|
<p className="px-3 pb-1 pt-3 text-xs font-semibold uppercase tracking-wide text-muted">All plants</p>
|
||||||
</>
|
</>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
{filtered.map(row)}
|
{filtered.map((p) => row(p, 'all'))}
|
||||||
|
|
||||||
{plants.isSuccess && filtered.length === 0 && (
|
{plants.isSuccess && filtered.length === 0 && (
|
||||||
<p className="p-4 text-sm text-muted">No plants match your search.</p>
|
<p className="p-4 text-sm text-muted">No plants match your search.</p>
|
||||||
|
|||||||
@@ -43,6 +43,20 @@ export function isBuiltin(p: Plant): boolean {
|
|||||||
return p.ownerId == null
|
return p.ownerId == null
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Category filter value: a specific category, or 'all'. Shared by the catalog
|
||||||
|
* page and the PlantPicker so their chip rows and filtering stay in sync. */
|
||||||
|
export type CategoryFilter = PlantCategory | 'all'
|
||||||
|
|
||||||
|
/** Filter a plant list by a name query (case-insensitive substring) and
|
||||||
|
* category. The single source of truth for both the /plants grid and the
|
||||||
|
* PlantPicker list. */
|
||||||
|
export function filterPlants(plants: Plant[], query: string, category: CategoryFilter): Plant[] {
|
||||||
|
const q = query.trim().toLowerCase()
|
||||||
|
return plants.filter(
|
||||||
|
(p) => (category === 'all' || p.category === category) && (q === '' || p.name.toLowerCase().includes(q)),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
const plantsKey = ['plants'] as const
|
const plantsKey = ['plants'] as const
|
||||||
|
|
||||||
export const plantsQueryOptions = queryOptions({
|
export const plantsQueryOptions = queryOptions({
|
||||||
|
|||||||
@@ -1,18 +1,16 @@
|
|||||||
import { useMemo, useState } from 'react'
|
import { useMemo, useState } from 'react'
|
||||||
import { Alert } from '@/components/ui/Alert'
|
import { Alert } from '@/components/ui/Alert'
|
||||||
import { Button } from '@/components/ui/Button'
|
import { Button } from '@/components/ui/Button'
|
||||||
import { cn } from '@/lib/cn'
|
|
||||||
import { fieldControlClass } from '@/components/ui/field'
|
import { fieldControlClass } from '@/components/ui/field'
|
||||||
import { toast } from '@/components/ui/toast'
|
import { toast } from '@/components/ui/toast'
|
||||||
|
import { CategoryChips } from '@/components/plants/CategoryChips'
|
||||||
import { PlantCard } from '@/components/plants/PlantCard'
|
import { PlantCard } from '@/components/plants/PlantCard'
|
||||||
import { PlantFormModal } from '@/components/plants/PlantFormModal'
|
import { PlantFormModal } from '@/components/plants/PlantFormModal'
|
||||||
import { DeletePlantModal } from '@/components/plants/DeletePlantModal'
|
import { DeletePlantModal } from '@/components/plants/DeletePlantModal'
|
||||||
import { PlantPicker } from '@/editor/PlantPicker'
|
import { PlantPicker } from '@/editor/PlantPicker'
|
||||||
import { CATEGORY_LABELS, PLANT_CATEGORIES, usePlants, type Plant, type PlantCategory } from '@/lib/plants'
|
import { filterPlants, usePlants, type CategoryFilter, type Plant } from '@/lib/plants'
|
||||||
import type { UnitPref } from '@/lib/units'
|
import type { UnitPref } from '@/lib/units'
|
||||||
|
|
||||||
type CategoryFilter = PlantCategory | 'all'
|
|
||||||
|
|
||||||
// Which modal is open, if any. edit/duplicate/delete carry the target plant.
|
// Which modal is open, if any. edit/duplicate/delete carry the target plant.
|
||||||
type Dialog =
|
type Dialog =
|
||||||
| { kind: 'create' }
|
| { kind: 'create' }
|
||||||
@@ -54,26 +52,7 @@ export function PlantsPage() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const all = useMemo(() => plants.data ?? [], [plants.data])
|
const all = useMemo(() => plants.data ?? [], [plants.data])
|
||||||
const filtered = useMemo(() => {
|
const filtered = useMemo(() => filterPlants(all, query, category), [all, query, category])
|
||||||
const q = query.trim().toLowerCase()
|
|
||||||
return all.filter(
|
|
||||||
(p) => (category === 'all' || p.category === category) && (q === '' || p.name.toLowerCase().includes(q)),
|
|
||||||
)
|
|
||||||
}, [all, query, category])
|
|
||||||
|
|
||||||
const chip = (value: CategoryFilter, label: string) => (
|
|
||||||
<button
|
|
||||||
key={value}
|
|
||||||
type="button"
|
|
||||||
onClick={() => setCategory(value)}
|
|
||||||
className={cn(
|
|
||||||
'shrink-0 rounded-full px-3 py-1 text-sm font-medium transition-colors',
|
|
||||||
category === value ? 'bg-accent text-accent-contrast' : 'bg-border/50 text-muted hover:text-fg',
|
|
||||||
)}
|
|
||||||
>
|
|
||||||
{label}
|
|
||||||
</button>
|
|
||||||
)
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<section>
|
<section>
|
||||||
@@ -104,9 +83,8 @@ export function PlantsPage() {
|
|||||||
aria-label="Search plants"
|
aria-label="Search plants"
|
||||||
className={fieldControlClass}
|
className={fieldControlClass}
|
||||||
/>
|
/>
|
||||||
<div className="flex gap-1.5 overflow-x-auto pb-1">
|
<div className="pb-1">
|
||||||
{chip('all', 'All')}
|
<CategoryChips value={category} onChange={setCategory} />
|
||||||
{PLANT_CATEGORIES.map((c) => chip(c, CATEGORY_LABELS[c]))}
|
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user