Cleanup: ConfirmModal primitive, drop dead PageStub, full-width editor (#107) #116

Merged
steve merged 2 commits from feat/cleanup into main 2026-07-22 14:05:20 +00:00
9 changed files with 178 additions and 184 deletions
-13
View File
@@ -1,13 +0,0 @@
import type { ReactNode } from 'react'
/** Placeholder page scaffolding used until each feature issue fills these in. */
export function PageStub({ title, children }: { title: string; children?: ReactNode }) {
return (
<section>
<h1 className="text-2xl font-semibold tracking-tight">{title}</h1>
<p className="mt-2 text-sm text-muted">
{children ?? 'Placeholder — this page arrives in a later issue.'}
</p>
</section>
)
}
@@ -1,42 +1,22 @@
import { useState } from 'react'
import { Modal } from '@/components/ui/Modal'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { errorMessage } from '@/lib/api'
import { ConfirmModal } from '@/components/ui/ConfirmModal'
import { useDeleteGarden, type Garden } from '@/lib/gardens'
/** Confirmation dialog for deleting a garden (and everything in it). */
export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) {
const deletion = useDeleteGarden()
const [error, setError] = useState<string | null>(null)
async function onConfirm() {
setError(null)
try {
await deletion.mutateAsync(garden.id)
onClose()
} catch (err) {
setError(errorMessage(err, 'Could not delete the garden.'))
}
}
return (
<Modal title="Delete garden" onClose={onClose} busy={deletion.isPending}>
<div className="flex flex-col gap-4">
<p className="text-sm text-muted">
Delete <span className="font-medium text-fg">{garden.name}</span> and everything planned in it?
This can't be undone.
</p>
{error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={deletion.isPending}>
Cancel
</Button>
<Button type="button" variant="danger" onClick={onConfirm} disabled={deletion.isPending}>
{deletion.isPending ? 'Deleting' : 'Delete'}
</Button>
</div>
</div>
</Modal>
<ConfirmModal
title="Delete garden"
confirmLabel="Delete"
busyLabel="Deleting…"
errorFallback="Could not delete the garden."
onConfirm={() => deletion.mutateAsync(garden.id)}
onClose={onClose}
>
<p className="text-sm text-muted">
Delete <span className="font-medium text-fg">{garden.name}</span> and everything planned in it?
This can't be undone.
</p>
</ConfirmModal>
)
}
+21 -35
View File
@@ -1,8 +1,4 @@
import { useState } from 'react'
import { Modal } from '@/components/ui/Modal'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { errorMessage } from '@/lib/api'
import { ConfirmModal } from '@/components/ui/ConfirmModal'
import { useMe } from '@/lib/auth'
import type { Garden } from '@/lib/gardens'
import { useRemoveShare } from '@/lib/shares'
@@ -12,36 +8,26 @@ import { useRemoveShare } from '@/lib/shares'
export function LeaveGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) {
const me = useMe()
const remove = useRemoveShare(garden.id)
const [error, setError] = useState<string | null>(null)
async function onConfirm() {
if (!me.data) return
setError(null)
try {
await remove.mutateAsync(me.data.id)
onClose()
} catch (err) {
setError(errorMessage(err, 'Could not leave the garden.'))
}
}
return (
<Modal title="Leave garden" onClose={onClose} busy={remove.isPending}>
<div className="flex flex-col gap-4">
<p className="text-sm text-muted">
Leave <span className="font-medium text-fg">{garden.name}</span>? You'll lose access until the owner
shares it with you again.
</p>
{error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={remove.isPending}>
Cancel
</Button>
<Button type="button" variant="danger" onClick={onConfirm} disabled={remove.isPending || !me.data}>
{remove.isPending ? 'Leaving' : 'Leave'}
</Button>
</div>
</div>
</Modal>
<ConfirmModal
title="Leave garden"
confirmLabel="Leave"
busyLabel="Leaving…"
confirmDisabled={!me.data}
errorFallback="Could not leave the garden."
onConfirm={async () => {
Review

🟡 Dialog closes on no-op when me.data is absent instead of staying open

error-handling · flagged by 3 models

  • LeaveGardenModal closes on no-op when me.data is absent. web/src/components/gardens/LeaveGardenModal.tsx:18

🪰 Gadfly · advisory

🟡 **Dialog closes on no-op when me.data is absent instead of staying open** _error-handling · flagged by 3 models_ - **`LeaveGardenModal` closes on no-op when `me.data` is absent.** `web/src/components/gardens/LeaveGardenModal.tsx:18` <sub>🪰 Gadfly · advisory</sub>
// The button is disabled without a current user; throw rather than
// silently resolve (which would close the dialog as if it had worked) if
// that guard ever drifts.
if (!me.data) throw new Error('Not signed in.')
await remove.mutateAsync(me.data.id)
}}
onClose={onClose}
>
<p className="text-sm text-muted">
Leave <span className="font-medium text-fg">{garden.name}</span>? You'll lose access until the owner
shares it with you again.
</p>
</ConfirmModal>
)
}
+11 -3
View File
@@ -39,14 +39,19 @@ export function AppShell() {
// so the '/gardens' list itself still shows the bar.
const inEditor = !!matchRoute({ to: '/gardens/$gardenId', fuzzy: false })
const inPublicGarden = !!matchRoute({ to: '/g/$token', fuzzy: false })
const showBottomNav = !!user && !inEditor && !inPublicGarden
// A canvas route wants the whole width — the garden editor is squeezed by the
// max-w-5xl reading measure the other pages use (#107).
const canvasRoute = inEditor || inPublicGarden
const showBottomNav = !!user && !canvasRoute
const visibleSections = sections.filter((s) => !s.adminOnly || user?.isAdmin)
return (
<div className="flex min-h-full flex-col">
<header className="sticky top-0 z-20 border-b border-border bg-surface/90 backdrop-blur">
<nav className="mx-auto flex max-w-5xl items-center gap-4 px-4 py-3">
{/* The bar matches the content width below: constrained on reading pages,
edge-to-edge on the canvas routes so the brand aligns with the editor. */}
<nav className={cn('flex items-center gap-4 px-4 py-3', canvasRoute ? '' : 'mx-auto max-w-5xl')}>
Review

Ternary-to-empty-string vs && idiom used inconsistently for the same canvasRoute condition in one file

maintainability · flagged by 1 model

  • web/src/components/layout/AppShell.tsx:54 vs :98 — the nav className uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while <main> (44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identical canvasRoute condition. cn() strips falsy values, so the ternary's '' branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…

🪰 Gadfly · advisory

⚪ **Ternary-to-empty-string vs && idiom used inconsistently for the same canvasRoute condition in one file** _maintainability · flagged by 1 model_ - `web/src/components/layout/AppShell.tsx:54` vs `:98` — the `nav` className uses a ternary-to-empty-string (`canvasRoute ? '' : 'mx-auto max-w-5xl'`) while `<main>` (44 lines below) uses a short-circuit (`!canvasRoute && 'mx-auto max-w-5xl'`) for the identical `canvasRoute` condition. `cn()` strips falsy values, so the ternary's `''` branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise… <sub>🪰 Gadfly · advisory</sub>
<Link to="/gardens" className="text-lg font-semibold text-accent-strong">
🌱 pansy
</Link>
@@ -87,7 +92,10 @@ export function AppShell() {
<main
className={cn(
'mx-auto w-full max-w-5xl flex-1 px-4 py-6',
'w-full flex-1 px-4 py-6',
// Constrain the reading pages to a comfortable measure; the canvas
// routes go edge-to-edge so the garden gets the whole screen.
!canvasRoute && 'mx-auto max-w-5xl',
// Clear the fixed bottom bar on mobile so content isn't hidden behind
// it. The 3.5rem must match BottomNav's h-14 (kept adjacent below).
showBottomNav && 'pb-[calc(3.5rem+env(safe-area-inset-bottom))] md:pb-6',
+16 -36
View File
@@ -1,46 +1,26 @@
import { useState } from 'react'
import { Modal } from '@/components/ui/Modal'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { errorMessage } from '@/lib/api'
import { ConfirmModal } from '@/components/ui/ConfirmModal'
import { useDeletePlant, type Plant } from '@/lib/plants'
/**
* Confirmation dialog for deleting a custom plant. A plant still used by
* plantings is refused by the server (409 PLANT_IN_USE); we surface that
* message inline rather than pretending it worked.
* plantings is refused by the server (409 PLANT_IN_USE); ConfirmModal surfaces
* that message inline rather than pretending it worked.
*/
export function DeletePlantModal({ plant, onClose }: { plant: Plant; onClose: () => void }) {
const deletion = useDeletePlant()
const [error, setError] = useState<string | null>(null)
async function onConfirm() {
setError(null)
try {
await deletion.mutateAsync(plant.id)
onClose()
} catch (err) {
setError(errorMessage(err, 'Could not delete the plant.'))
}
}
return (
<Modal title="Delete plant" onClose={onClose} busy={deletion.isPending}>
<div className="flex flex-col gap-4">
<p className="text-sm text-muted">
Delete <span className="font-medium text-fg">{plant.name}</span> from your catalog? This can't be
undone.
</p>
{error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={deletion.isPending}>
Cancel
</Button>
<Button type="button" variant="danger" onClick={onConfirm} disabled={deletion.isPending}>
{deletion.isPending ? 'Deleting' : 'Delete'}
</Button>
</div>
</div>
</Modal>
<ConfirmModal
title="Delete plant"
confirmLabel="Delete"
busyLabel="Deleting…"
errorFallback="Could not delete the plant."
onConfirm={() => deletion.mutateAsync(plant.id)}
onClose={onClose}
>
<p className="text-sm text-muted">
Delete <span className="font-medium text-fg">{plant.name}</span> from your catalog? This can't be
undone.
</p>
</ConfirmModal>
)
}
@@ -1,8 +1,4 @@
import { useState } from 'react'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { Modal } from '@/components/ui/Modal'
import { errorMessage } from '@/lib/api'
import { ConfirmModal } from '@/components/ui/ConfirmModal'
import { formatQuantity, useDeleteSeedLot, type SeedLot } from '@/lib/seedLots'
/**
@@ -12,39 +8,23 @@ import { formatQuantity, useDeleteSeedLot, type SeedLot } from '@/lib/seedLots'
*/
export function DeleteSeedLotModal({ lot, onClose }: { lot: SeedLot; onClose: () => void }) {
const del = useDeleteSeedLot()
const [error, setError] = useState<string | null>(null)
return (
<Modal title="Retire this seed lot?" onClose={onClose} busy={del.isPending}>
<div className="flex flex-col gap-3">
<p className="text-sm text-fg">
{formatQuantity(lot.quantity)} {lot.unit}
{lot.vendor ? ` from ${lot.vendor}` : ''}
{lot.packedForYear != null ? `, packed for ${lot.packedForYear}` : ''}.
</p>
<p className="text-sm text-muted">
Anything planted from it stays exactly where it is it just stops being attributed to this purchase.
</p>
{error && <Alert>{error}</Alert>}
<div className="mt-1 flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={del.isPending}>
Cancel
</Button>
<Button
type="button"
variant="danger"
disabled={del.isPending}
onClick={() =>
del.mutate(lot.id, {
onSuccess: onClose,
onError: (err) => setError(errorMessage(err, 'Could not retire the lot.')),
})
}
>
{del.isPending ? 'Retiring…' : 'Retire lot'}
</Button>
</div>
</div>
</Modal>
<ConfirmModal
title="Retire this seed lot?"
confirmLabel="Retire lot"
busyLabel="Retiring…"
errorFallback="Could not retire the lot."
onConfirm={() => del.mutateAsync(lot.id)}
onClose={onClose}
>
<p className="text-sm text-fg">
{formatQuantity(lot.quantity)} {lot.unit}
{lot.vendor ? ` from ${lot.vendor}` : ''}
{lot.packedForYear != null ? `, packed for ${lot.packedForYear}` : ''}.
</p>
<p className="text-sm text-muted">
Anything planted from it stays exactly where it is it just stops being attributed to this purchase.
</p>
</ConfirmModal>
)
}
+79
View File
@@ -0,0 +1,79 @@
import { useState, type ReactNode } from 'react'
import { Modal } from '@/components/ui/Modal'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { errorMessage } from '@/lib/api'
/**
* A confirm-and-act dialog: a message, then Cancel / Confirm. It owns the shared
* shape every confirmation repeated by hand — the busy lock, the inline error on
* failure (so a 409 like PLANT_IN_USE is shown, not swallowed), and the footer —
* so each caller supplies only its message, its action, and its labels.
*
* onConfirm runs the action; it resolving closes the dialog, it throwing keeps
* the dialog open with the error and re-enables the button for a retry. This is
* confirmations only — dialogs with their own inputs (a rename, a form) keep
* using Modal directly.
*/
export function ConfirmModal({
title,
children,
confirmLabel,
busyLabel,
confirmVariant = 'danger',
confirmDisabled = false,
errorFallback,
onConfirm,
onClose,
}: {
title: string
/** The body — what's being confirmed. */
children: ReactNode
confirmLabel: string
/** Label while the action is in flight (e.g. "Deleting…"). */
Review

confirmVariant 'primary' option is unused by any caller — speculative API surface on a new primitive

maintainability · flagged by 1 model

  • web/src/components/layout/AppShell.tsx:54 vs :98 — the nav className uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while <main> (44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identical canvasRoute condition. cn() strips falsy values, so the ternary's '' branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…

🪰 Gadfly · advisory

⚪ **confirmVariant 'primary' option is unused by any caller — speculative API surface on a new primitive** _maintainability · flagged by 1 model_ - `web/src/components/layout/AppShell.tsx:54` vs `:98` — the `nav` className uses a ternary-to-empty-string (`canvasRoute ? '' : 'mx-auto max-w-5xl'`) while `<main>` (44 lines below) uses a short-circuit (`!canvasRoute && 'mx-auto max-w-5xl'`) for the identical `canvasRoute` condition. `cn()` strips falsy values, so the ternary's `''` branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise… <sub>🪰 Gadfly · advisory</sub>
busyLabel: string
confirmVariant?: 'danger' | 'primary'
/** Extra guard beyond busy (e.g. nothing to clear, no current user). */
confirmDisabled?: boolean
/** Message if the action throws something without its own user-facing text. */
errorFallback: string
onConfirm: () => Promise<unknown>
onClose: () => void
}) {
const [busy, setBusy] = useState(false)
const [error, setError] = useState<string | null>(null)
async function handleConfirm() {
setError(null)
setBusy(true)
try {
await onConfirm()
onClose()
} catch (err) {
setError(errorMessage(err, errorFallback))
setBusy(false) // keep the dialog open so the message shows and retry works
}
}
return (
<Modal title={title} onClose={onClose} busy={busy}>
<div className="flex flex-col gap-4">
{children}
{error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={busy}>
Cancel
</Button>
<Button
type="button"
variant={confirmVariant}
onClick={handleConfirm}
disabled={busy || confirmDisabled}
>
{busy ? busyLabel : confirmLabel}
</Button>
</div>
</div>
</Modal>
)
}
+16 -24
View File
@@ -1,5 +1,4 @@
import { Modal } from '@/components/ui/Modal'
import { Button } from '@/components/ui/Button'
import { ConfirmModal } from '@/components/ui/ConfirmModal'
import { useClearObject } from '@/lib/objects'
/** Confirm clearing every active plop from a focused bed (soft-remove — the rows
@@ -19,27 +18,20 @@ export function ClearBedModal({
}) {
const clear = useClearObject(gardenId)
return (
<Modal title="Clear bed" onClose={onClose} busy={clear.isPending}>
<div className="flex flex-col gap-4">
<p className="text-sm text-muted">
Remove all <span className="font-medium text-fg">{plopCount}</span>{' '}
{plopCount === 1 ? 'plant' : 'plants'} from{' '}
<span className="font-medium text-fg">{objectName}</span>? They're marked removed but kept in history.
</p>
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={clear.isPending}>
Cancel
</Button>
<Button
type="button"
variant="danger"
disabled={clear.isPending || plopCount === 0}
onClick={() => clear.mutate(objectId, { onSuccess: onClose })}
>
{clear.isPending ? 'Clearing' : 'Clear bed'}
</Button>
</div>
</div>
</Modal>
<ConfirmModal
Review

🟠 Duplicate error notification: ClearBedModal inline error + useClearObject toast on mutation failure

correctness · flagged by 2 models

  • web/src/editor/ClearBedModal.tsx:21 — The refactor switches ClearBedModal from clear.mutate() with an onSuccess callback to clear.mutateAsync() inside ConfirmModal. The underlying useClearObject hook (web/src/lib/objects.ts:384) still defines onError: (err) => toast.error(...). TanStack Query runs onError before mutateAsync rejects, so a failure now produces two notifications: the inline error from ConfirmModal and a toast from the mutation hook. Previously th…

🪰 Gadfly · advisory

🟠 **Duplicate error notification: ClearBedModal inline error + useClearObject toast on mutation failure** _correctness · flagged by 2 models_ - **`web/src/editor/ClearBedModal.tsx:21`** — The refactor switches `ClearBedModal` from `clear.mutate()` with an `onSuccess` callback to `clear.mutateAsync()` inside `ConfirmModal`. The underlying `useClearObject` hook (`web/src/lib/objects.ts:384`) still defines `onError: (err) => toast.error(...)`. TanStack Query runs `onError` before `mutateAsync` rejects, so a failure now produces **two notifications**: the inline error from `ConfirmModal` *and* a toast from the mutation hook. Previously th… <sub>🪰 Gadfly · advisory</sub>
title="Clear bed"
confirmLabel="Clear bed"
busyLabel="Clearing…"
confirmDisabled={plopCount === 0}
errorFallback="Could not clear the bed."
Review

🟡 ClearBed now double-reports errors (inline Alert + toast from useClearObject.onError)

correctness, error-handling · flagged by 3 models

  • Double error reporting in ClearBedModalweb/src/editor/ClearBedModal.tsx:26 (via web/src/components/ui/ConfirmModal.tsx). useClearObject still carries onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')) (web/src/lib/objects.ts:384), so on a clear failure the user now sees both the inline Alert that ConfirmModal renders and the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a glo…

🪰 Gadfly · advisory

🟡 **ClearBed now double-reports errors (inline Alert + toast from useClearObject.onError)** _correctness, error-handling · flagged by 3 models_ - **Double error reporting in `ClearBedModal`** — `web/src/editor/ClearBedModal.tsx:26` (via `web/src/components/ui/ConfirmModal.tsx`). `useClearObject` still carries `onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.'))` (`web/src/lib/objects.ts:384`), so on a clear failure the user now sees both the inline `Alert` that `ConfirmModal` renders *and* the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a glo… <sub>🪰 Gadfly · advisory</sub>
onConfirm={() => clear.mutateAsync(objectId)}
onClose={onClose}
>
<p className="text-sm text-muted">
Remove all <span className="font-medium text-fg">{plopCount}</span>{' '}
{plopCount === 1 ? 'plant' : 'plants'} from{' '}
<span className="font-medium text-fg">{objectName}</span>? They're marked removed but kept in history.
</p>
</ConfirmModal>
)
}
+3 -1
View File
@@ -381,7 +381,9 @@ export function useClearObject(gardenId: number) {
return res.cleared
},
onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),
onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')),
// No toast: the only caller (ClearBedModal → ConfirmModal) shows the failure
Review

🟡 ClearBed failure now surfaces both a toast and ConfirmModal's inline alert

error-handling · flagged by 1 model

  • web/src/lib/objects.ts:384 / web/src/editor/ClearBedModal.tsxuseClearObject still has its own onError: (err) => toast.error(...). Since ClearBedModal now uses ConfirmModal, whose handleConfirm also catches the same rejection from clear.mutateAsync(objectId) and renders it inline (ConfirmModal.tsx:52-54), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (useDeleteGarden, `useDele…

🪰 Gadfly · advisory

🟡 **ClearBed failure now surfaces both a toast and ConfirmModal's inline alert** _error-handling · flagged by 1 model_ - **`web/src/lib/objects.ts:384` / `web/src/editor/ClearBedModal.tsx`** — `useClearObject` still has its own `onError: (err) => toast.error(...)`. Since `ClearBedModal` now uses `ConfirmModal`, whose `handleConfirm` also catches the same rejection from `clear.mutateAsync(objectId)` and renders it inline (`ConfirmModal.tsx:52-54`), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (`useDeleteGarden`, `useDele… <sub>🪰 Gadfly · advisory</sub>
// inline in the dialog, which is more contextual than a detached toast — and
// two of them for one failure is worse than one.
})
}