Address Gadfly review on #17: share-response bug + error surfacing

Critical: - useAddShare/useUpdateShareRole no longer parse the response with the list's Share schema — the backend returns a bare GardenShare (no email/displayName), so every successful invite/role-change was throwing and showing a false error. onSuccess refetches the list, which has the full data. - ShareGardenModal surfaces role-change / remove failures (mutate onError → the dialog's Alert) instead of swallowing them; Modal busy now covers all three pending states. Robustness / defense in depth: - Inspector/PlopInspector patch() early-returns when readOnly, so a blur can't fire a mutation if the role was downgraded mid-edit. - onPickPlant guards canEdit. - Ownership is now the authoritative ownerId==me check (GardenCard via a currentUserId prop, GardenEditorPage via useMe), so a missing my_role can never lock an owner out or show them a Leave button. Cleanup: - Shared cardActionClass/cardDangerClass (GardenCard + PlantCard). - Normalized the read-only <fieldset> indentation; dropped the dead useShares `enabled` param. tsc --noEmit clean; 24/24 vitest; production build green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
2026-07-19 00:12:54 -04:00
parent be5ed18db0
commit 85c355ce4d
9 changed files with 196 additions and 172 deletions
@@ -1,33 +1,30 @@
 import { Link } from '@tanstack/react-router'
-import { isOwnerRole, type Garden } from '@/lib/gardens'
+import type { Garden } from '@/lib/gardens'
 import { formatDimensions } from '@/lib/units'
-
-const actionClass =
-  'rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors ' +
-  'hover:bg-border/50 hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40'
-const dangerClass =
-  'rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors ' +
-  'hover:bg-red-500/10 hover:text-red-600 focus-visible:ring-2 focus-visible:ring-red-500/40 dark:hover:text-red-400'
+import { cardActionClass, cardDangerClass } from '@/components/ui/cardActions'

 /**
 * One garden as a card: the body links into the editor. The footer differs by
 * role — the owner gets Share / Edit / Delete; a recipient sees a "shared · role"
 * badge and a Leave action (garden metadata edit + sharing are owner-only).
+ * Ownership is the authoritative ownerId==me check, not the my_role hint.
 */
 export function GardenCard({
  garden,
+  currentUserId,
  onShare,
  onEdit,
  onDelete,
  onLeave,
 }: {
  garden: Garden
+  currentUserId?: number
  onShare: () => void
  onEdit: () => void
  onDelete: () => void
  onLeave: () => void
 }) {
-  const owner = isOwnerRole(garden.myRole)
+  const owner = currentUserId != null && garden.ownerId === currentUserId
  return (
    <div className="flex flex-col rounded-xl border border-border bg-surface transition-colors hover:border-accent/50">
      <Link
@@ -51,18 +48,18 @@ export function GardenCard({
      <div className="flex justify-end gap-1 border-t border-border px-2 py-1.5">
        {owner ? (
          <>
-            <button type="button" onClick={onShare} className={actionClass}>
+            <button type="button" onClick={onShare} className={cardActionClass}>
              Share
            </button>
-            <button type="button" onClick={onEdit} className={actionClass}>
+            <button type="button" onClick={onEdit} className={cardActionClass}>
              Edit
            </button>
-            <button type="button" onClick={onDelete} className={dangerClass}>
+            <button type="button" onClick={onDelete} className={cardDangerClass}>
              Delete
            </button>
          </>
        ) : (
-          <button type="button" onClick={onLeave} className={dangerClass}>
+          <button type="button" onClick={onLeave} className={cardDangerClass}>
            Leave
          </button>
        )}
@@ -46,8 +46,10 @@ export function ShareGardenModal({ garden, onClose }: { garden: Garden; onClose:
    }
  }

+  const onMutationError = (fallback: string) => (err: unknown) => setError(errorMessage(err, fallback))
+
  return (
-    <Modal title="Share garden" onClose={onClose} busy={add.isPending}>
+    <Modal title="Share garden" onClose={onClose} busy={add.isPending || updateRole.isPending || remove.isPending}>
      <div className="flex flex-col gap-4">
        <form onSubmit={onInvite} className="flex flex-col gap-2">
          <TextField
@@ -90,7 +92,13 @@ export function ShareGardenModal({ garden, onClose }: { garden: Garden; onClose:
                </div>
                <select
                  value={sh.role}
-                  onChange={(e) => updateRole.mutate({ userId: sh.userId, role: e.target.value as ShareRole })}
+                  onChange={(e) => {
+                    setError(null)
+                    updateRole.mutate(
+                      { userId: sh.userId, role: e.target.value as ShareRole },
+                      { onError: onMutationError('Could not change that role.') },
+                    )
+                  }}
                  aria-label={`Role for ${sh.displayName}`}
                  className={cn(fieldControlClass, 'w-auto px-2 py-1 text-sm')}
                >
@@ -99,7 +107,10 @@ export function ShareGardenModal({ garden, onClose }: { garden: Garden; onClose:
                </select>
                <button
                  type="button"
-                  onClick={() => remove.mutate(sh.userId)}
+                  onClick={() => {
+                    setError(null)
+                    remove.mutate(sh.userId, { onError: onMutationError('Could not remove that person.') })
+                  }}
                  aria-label={`Remove ${sh.displayName}`}
                  className="rounded-md px-2 py-1 text-sm text-muted transition-colors hover:bg-red-500/10 hover:text-red-600 dark:hover:text-red-400"
                >
@@ -1,14 +1,8 @@
 import { PlantIcon } from './PlantIcon'
+import { cardActionClass, cardDangerClass } from '@/components/ui/cardActions'
 import { CATEGORY_LABELS, isBuiltin, type Plant } from '@/lib/plants'
 import { formatSpacing, type UnitPref } from '@/lib/units'

-const actionClass =
-  'rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors ' +
-  'hover:bg-border/50 hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40'
-const dangerClass =
-  'rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors ' +
-  'hover:bg-red-500/10 hover:text-red-600 focus-visible:ring-2 focus-visible:ring-red-500/40 dark:hover:text-red-400'
-
 /**
 * One catalog plant as a card: icon tile tinted with the plant's color, name,
 * category + mature spacing (unit-aware), and actions. Built-ins are badged and
@@ -53,16 +47,16 @@ export function PlantCard({
        />
      </div>
      <div className="flex justify-end gap-1 border-t border-border px-2 py-1.5">
-        <button type="button" onClick={onDuplicate} className={actionClass}>
+        <button type="button" onClick={onDuplicate} className={cardActionClass}>
          Duplicate
        </button>
        {!builtin && (
-          <button type="button" onClick={onEdit} className={actionClass}>
+          <button type="button" onClick={onEdit} className={cardActionClass}>
            Edit
          </button>
        )}
        {!builtin && (
-          <button type="button" onClick={onDelete} className={dangerClass}>
+          <button type="button" onClick={onDelete} className={cardDangerClass}>
            Delete
          </button>
        )}
@@ -0,0 +1,10 @@
+// Shared footer-action button styling for list cards (garden/plant), so the
+// verbatim class strings don't drift between them.
+
+export const cardActionClass =
+  'rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors ' +
+  'hover:bg-border/50 hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40'
+
+export const cardDangerClass =
+  'rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors ' +
+  'hover:bg-red-500/10 hover:text-red-600 focus-visible:ring-2 focus-visible:ring-red-500/40 dark:hover:text-red-400'
@@ -66,8 +66,10 @@ export function Inspector({
    setColor(object.color ?? DEFAULT_COLOR)
  }, [object.version, object.widthCm, object.heightCm, object.xCm, object.yCm, object.rotationDeg, object.name, object.notes, object.color, unit])

-  const patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) =>
+  const patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) => {
+    if (readOnly) return // a blur mustn't fire a mutation if the role changed mid-edit
    update.mutate({ id: object.id, version: object.version, ...fields })
+  }

  // Commit a dimension/position field. `positive` gates width/height (must be
  // ≥ the server minimum) but not x/y, which may be zero or negative. Compare at
@@ -119,111 +121,111 @@ export function Inspector({
          onBlur={() => name !== object.name && patch({ name })}
        />

-      <div className="grid grid-cols-2 gap-2">
-        <TextField
-          label={`Width (${u})`}
-          name="width"
-          type="number"
-          inputMode="decimal"
-          step="any"
-          value={width}
-          onChange={(e) => setWidth(e.target.value)}
-          onBlur={() => commitDim(width, object.widthCm, (cm) => patch({ widthCm: cm }), true)}
-        />
-        <TextField
-          label={`Height (${u})`}
-          name="height"
-          type="number"
-          inputMode="decimal"
-          step="any"
-          value={height}
-          onChange={(e) => setHeight(e.target.value)}
-          onBlur={() => commitDim(height, object.heightCm, (cm) => patch({ heightCm: cm }), true)}
-        />
-        <TextField
-          label={`X (${u})`}
-          name="x"
-          type="number"
-          inputMode="decimal"
-          step="any"
-          value={x}
-          onChange={(e) => setX(e.target.value)}
-          onBlur={() => commitDim(x, object.xCm, (cm) => patch({ xCm: cm }))}
-        />
-        <TextField
-          label={`Y (${u})`}
-          name="y"
-          type="number"
-          inputMode="decimal"
-          step="any"
-          value={y}
-          onChange={(e) => setY(e.target.value)}
-          onBlur={() => commitDim(y, object.yCm, (cm) => patch({ yCm: cm }))}
-        />
-      </div>
-
-      <TextField
-        label="Rotation (°)"
-        name="rotation"
-        type="number"
-        inputMode="numeric"
-        step="1"
-        value={rotation}
-        onChange={(e) => setRotation(e.target.value)}
-        onBlur={() => {
-          const v = parseFloat(rotation)
-          if (Number.isFinite(v) && v !== object.rotationDeg) patch({ rotationDeg: v })
-        }}
-      />
-
-      <div className="flex items-end gap-2">
-        <div className="flex flex-col gap-1.5">
-          <label htmlFor="obj-color" className="text-sm font-medium text-fg">
-            Color
-          </label>
-          <input
-            id="obj-color"
-            type="color"
-            value={color}
-            // onChange fires continuously while dragging in the native picker;
-            // track it locally for the live swatch and commit one PATCH on blur.
-            onChange={(e: ChangeEvent<HTMLInputElement>) => setColor(e.target.value)}
-            onBlur={() => color !== (object.color ?? DEFAULT_COLOR) && patch({ color })}
-            className="h-9 w-14 cursor-pointer rounded-md border border-border bg-surface"
+        <div className="grid grid-cols-2 gap-2">
+          <TextField
+            label={`Width (${u})`}
+            name="width"
+            type="number"
+            inputMode="decimal"
+            step="any"
+            value={width}
+            onChange={(e) => setWidth(e.target.value)}
+            onBlur={() => commitDim(width, object.widthCm, (cm) => patch({ widthCm: cm }), true)}
+          />
+          <TextField
+            label={`Height (${u})`}
+            name="height"
+            type="number"
+            inputMode="decimal"
+            step="any"
+            value={height}
+            onChange={(e) => setHeight(e.target.value)}
+            onBlur={() => commitDim(height, object.heightCm, (cm) => patch({ heightCm: cm }), true)}
+          />
+          <TextField
+            label={`X (${u})`}
+            name="x"
+            type="number"
+            inputMode="decimal"
+            step="any"
+            value={x}
+            onChange={(e) => setX(e.target.value)}
+            onBlur={() => commitDim(x, object.xCm, (cm) => patch({ xCm: cm }))}
+          />
+          <TextField
+            label={`Y (${u})`}
+            name="y"
+            type="number"
+            inputMode="decimal"
+            step="any"
+            value={y}
+            onChange={(e) => setY(e.target.value)}
+            onBlur={() => commitDim(y, object.yCm, (cm) => patch({ yCm: cm }))}
          />
        </div>
-        {object.color && (
-          <Button
-            variant="ghost"
-            className="px-2 py-1.5 text-xs"
-            onClick={() => {
-              setColor(DEFAULT_COLOR)
-              patch({ color: null })
-            }}
-          >
-            Clear
-          </Button>
-        )}
-      </div>

-      <label className="flex items-center gap-2 text-sm text-fg">
-        <input
-          type="checkbox"
-          checked={object.plantable}
-          onChange={(e) => patch({ plantable: e.target.checked })}
-          className="h-4 w-4 rounded border-border"
+        <TextField
+          label="Rotation (°)"
+          name="rotation"
+          type="number"
+          inputMode="numeric"
+          step="1"
+          value={rotation}
+          onChange={(e) => setRotation(e.target.value)}
+          onBlur={() => {
+            const v = parseFloat(rotation)
+            if (Number.isFinite(v) && v !== object.rotationDeg) patch({ rotationDeg: v })
+          }}
        />
-        Plantable
-      </label>

-        <TextArea
-          label="Notes"
-          name="notes"
-          rows={2}
-          value={notes}
-          onChange={(e) => setNotes(e.target.value)}
-          onBlur={() => notes !== object.notes && patch({ notes })}
-        />
+        <div className="flex items-end gap-2">
+          <div className="flex flex-col gap-1.5">
+            <label htmlFor="obj-color" className="text-sm font-medium text-fg">
+              Color
+            </label>
+            <input
+              id="obj-color"
+              type="color"
+              value={color}
+              // onChange fires continuously while dragging in the native picker;
+              // track it locally for the live swatch and commit one PATCH on blur.
+              onChange={(e: ChangeEvent<HTMLInputElement>) => setColor(e.target.value)}
+              onBlur={() => color !== (object.color ?? DEFAULT_COLOR) && patch({ color })}
+              className="h-9 w-14 cursor-pointer rounded-md border border-border bg-surface"
+            />
+          </div>
+          {object.color && (
+            <Button
+              variant="ghost"
+              className="px-2 py-1.5 text-xs"
+              onClick={() => {
+                setColor(DEFAULT_COLOR)
+                patch({ color: null })
+              }}
+            >
+              Clear
+            </Button>
+          )}
+        </div>
+
+        <label className="flex items-center gap-2 text-sm text-fg">
+          <input
+            type="checkbox"
+            checked={object.plantable}
+            onChange={(e) => patch({ plantable: e.target.checked })}
+            className="h-4 w-4 rounded border-border"
+          />
+          Plantable
+        </label>
+
+          <TextArea
+            label="Notes"
+            name="notes"
+            rows={2}
+            value={notes}
+            onChange={(e) => setNotes(e.target.value)}
+            onBlur={() => notes !== object.notes && patch({ notes })}
+          />
      </fieldset>

      {!readOnly &&
@@ -58,8 +58,10 @@ export function PlopInspector({
    setPlanted(p.plantedAt ?? '')
  }, [p.version, p.radiusCm, p.count, p.label, p.plantedAt, unit])

-  const patch = (fields: Omit<Parameters<typeof update.mutate>[0], 'id' | 'version'>) =>
+  const patch = (fields: Omit<Parameters<typeof update.mutate>[0], 'id' | 'version'>) => {
+    if (readOnly) return // a blur mustn't fire a mutation if the role changed mid-edit
    update.mutate({ id: plop.id, version: plop.version, ...fields })
+  }

  const u = spacingUnitLabel(unit)
  const derived = plant ? computeDerivedCount(p.radiusCm, plant.spacingCm) : p.derivedCount
@@ -125,40 +127,40 @@ export function PlopInspector({
      </div>

      <fieldset disabled={readOnly} className="flex min-w-0 flex-col gap-3 border-0 p-0">
-      <div className="grid grid-cols-2 gap-2">
-        <TextField
-          label={`Radius (${u})`}
-          name="radius"
-          type="number"
-          inputMode="decimal"
-          step="any"
-          min="0"
-          value={radius}
-          onChange={(e) => setRadius(e.target.value)}
-          onBlur={commitRadius}
-        />
-        <TextField
-          label="Count"
-          name="count"
-          type="number"
-          inputMode="numeric"
-          step="1"
-          min="1"
-          placeholder={`${derived} (auto)`}
-          value={count}
-          onChange={(e) => setCount(e.target.value)}
-          onBlur={commitCount}
-          hint={p.count != null ? 'Override — clear for auto' : 'Auto from area ÷ spacing²'}
-        />
-      </div>
+        <div className="grid grid-cols-2 gap-2">
+          <TextField
+            label={`Radius (${u})`}
+            name="radius"
+            type="number"
+            inputMode="decimal"
+            step="any"
+            min="0"
+            value={radius}
+            onChange={(e) => setRadius(e.target.value)}
+            onBlur={commitRadius}
+          />
+          <TextField
+            label="Count"
+            name="count"
+            type="number"
+            inputMode="numeric"
+            step="1"
+            min="1"
+            placeholder={`${derived} (auto)`}
+            value={count}
+            onChange={(e) => setCount(e.target.value)}
+            onBlur={commitCount}
+            hint={p.count != null ? 'Override — clear for auto' : 'Auto from area ÷ spacing²'}
+          />
+        </div>

-      <TextField
-        label="Label (optional)"
-        name="label"
-        value={label}
-        onChange={(e) => setLabel(e.target.value)}
-        onBlur={() => label !== (p.label ?? '') && patch({ label: label.trim() || null })}
-      />
+        <TextField
+          label="Label (optional)"
+          name="label"
+          value={label}
+          onChange={(e) => setLabel(e.target.value)}
+          onBlur={() => label !== (p.label ?? '') && patch({ label: label.trim() || null })}
+        />

        <TextField
          label="Planted"
@@ -31,16 +31,17 @@ export function sharesQueryOptions(gardenId: number) {
  })
 }

-/** The shares on a garden (owner-only endpoint; pass enabled=false to skip). */
-export function useShares(gardenId: number, enabled = true) {
-  return useQuery({ ...sharesQueryOptions(gardenId), enabled })
+/** The shares on a garden (owner-only endpoint). */
+export function useShares(gardenId: number) {
+  return useQuery(sharesQueryOptions(gardenId))
 }

+// The add/update responses are a bare GardenShare (no email/displayName), so we
+// don't parse them into the list's Share shape; onSuccess refetches the list.
 export function useAddShare(gardenId: number) {
  const qc = useQueryClient()
  return useMutation({
-    mutationFn: async (input: { email: string; role: ShareRole }): Promise<Share> =>
-      shareSchema.parse(await api.post(`/gardens/${gardenId}/shares`, input)),
+    mutationFn: (input: { email: string; role: ShareRole }) => api.post(`/gardens/${gardenId}/shares`, input),
    onSuccess: () => qc.invalidateQueries({ queryKey: sharesKey(gardenId) }),
  })
 }
@@ -48,8 +49,8 @@ export function useAddShare(gardenId: number) {
 export function useUpdateShareRole(gardenId: number) {
  const qc = useQueryClient()
  return useMutation({
-    mutationFn: async ({ userId, role }: { userId: number; role: ShareRole }): Promise<Share> =>
-      shareSchema.parse(await api.patch(`/gardens/${gardenId}/shares/${userId}`, { role })),
+    mutationFn: ({ userId, role }: { userId: number; role: ShareRole }) =>
+      api.patch(`/gardens/${gardenId}/shares/${userId}`, { role }),
    onSuccess: () => qc.invalidateQueries({ queryKey: sharesKey(gardenId) }),
  })
 }
@@ -11,7 +11,7 @@ import { kindDef } from '@/editor/kinds'
 import { useEditorStore } from '@/editor/store'
 import type { EditorGarden } from '@/editor/types'
 import { ShareGardenModal } from '@/components/gardens/ShareGardenModal'
-import { canEditRole, isOwnerRole } from '@/lib/gardens'
+import { useMe } from '@/lib/auth'
 import { toEditorObject, useGardenFull, useUpdatePlanting } from '@/lib/objects'
 import { toEditorPlanting } from '@/lib/plantings'

@@ -23,6 +23,7 @@ export function GardenEditorPage() {
  const { focus } = routeApi.useSearch()
  const navigate = routeApi.useNavigate()
  const full = useGardenFull(gid)
+  const me = useMe()

  const selectedId = useEditorStore((s) => s.selectedId)
  const select = useEditorStore((s) => s.select)
@@ -123,9 +124,11 @@ export function GardenEditorPage() {
    heightCm: g.heightCm,
    unitPref: g.unitPref,
  }
-  // Role-driven gating from the server's my_role — never guessed client-side.
-  const canEdit = canEditRole(g.myRole)
-  const isOwner = isOwnerRole(g.myRole)
+  // Role-driven gating. Ownership is the authoritative ownerId==me check (so a
+  // missing my_role can never lock an owner out); edit access is owner or an
+  // editor share.
+  const isOwner = me.data != null && g.ownerId === me.data.id
+  const canEdit = isOwner || g.myRole === 'editor'

  const selectedObject = objects.find((o) => o.id === selectedId) ?? null
  const focusedObject = focusedObjectId != null ? objects.find((o) => o.id === focusedObjectId) ?? null : null
@@ -133,6 +136,7 @@ export function GardenEditorPage() {
  const selectedPlop = plantings.find((p) => p.id === selectedPlantingId) ?? null

  function onPickPlant(plantId: number) {
+    if (!canEdit) return // defense in depth: viewers can't reach the picker anyway
    const plant = plantsById.get(plantId)
    if (!plant) return
    if (picker === 'change' && selectedPlop) {
@@ -6,6 +6,7 @@ import { GardenCard } from '@/components/gardens/GardenCard'
 import { GardenFormModal } from '@/components/gardens/GardenFormModal'
 import { LeaveGardenModal } from '@/components/gardens/LeaveGardenModal'
 import { ShareGardenModal } from '@/components/gardens/ShareGardenModal'
+import { useMe } from '@/lib/auth'
 import { useGardens, type Garden } from '@/lib/gardens'

 // Which modal is open, if any. edit/delete/share/leave carry the target garden.
@@ -19,6 +20,7 @@ type Dialog =

 export function GardensPage() {
  const gardens = useGardens()
+  const me = useMe()
  const [dialog, setDialog] = useState<Dialog>(null)
  const close = () => setDialog(null)
  const hasGardens = !!gardens.data && gardens.data.length > 0
@@ -50,6 +52,7 @@ export function GardensPage() {
              <GardenCard
                key={g.id}
                garden={g}
+                currentUserId={me.data?.id}
                onShare={() => setDialog({ kind: 'share', garden: g })}
                onEdit={() => setDialog({ kind: 'edit', garden: g })}
                onDelete={() => setDialog({ kind: 'delete', garden: g })}