Address Gadfly findings on #84

- Object aria-label uses kindDef().label (the canonical "In-ground") instead of an ad-hoc kind.replace() that produced "in ground" and diverged from the UI. 4+ models flagged this. - aria-current, not aria-pressed, for the selected object — selection isn't a toggle, which is what aria-pressed means; aria-current marks the active item. - Modal focus trap made robust: if focus is NOT inside the dialog (fell to <body> because the focused control was removed — ShareGardenModal's remove-share button — or disabled while busy, or externally stolen), Tab now pulls it back in instead of escaping. The previous branches only handled focus being exactly at a known boundary. - Focus restore checks opener.isConnected before calling focus(): the delete/ clear flows this targets often remove the element that opened the dialog, and a disconnected node's focus() silently no-ops. - Hoisted the focusable-element selector to a module constant, and excluded input[type="hidden"] (it matched input:not([disabled]) and, at a boundary, broke the wrap). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Make the canvas keyboard-reachable + trap focus in dialogs (#84 )
2026-07-21 23:33:38 -04:00 · 2026-07-21 23:20:47 -04:00 · 2026-07-21 22:56:01 -04:00 · 2026-07-22 02:53:55 +00:00 · 2026-07-22 02:53:27 +00:00 · 2026-07-22 02:53:13 +00:00
15 changed files with 915 additions and 44 deletions
@@ -129,21 +129,19 @@ fix what's real → merge when the pipeline is green. Do not grade Gadfly findin
 A push to `main` builds the image and deploys to Komodo; the live instance at
 `pansy.orgrimmar.dudenhoeffer.casa` updates a few minutes later.

-**Gadfly reviews the PR as opened, not as merged.** The workflow triggers on
-`opened`/`reopened`/`ready_for_review` — deliberately *not* `synchronize` — so
-every commit you push afterwards, including the ones you push in response to
-Gadfly itself, is unreviewed unless you ask. Once you've stopped pushing and
-before you merge, comment **`@gadfly review`** on the PR to re-trigger it. The
-phrase is required, and this is not hypothetical: on #76 the follow-up commit
-was the one that contained a real bug.
+**One sweep, not a loop.** Take Gadfly's *initial* review, fix what's real, and
+merge once the build is green. Do NOT re-trigger Gadfly on the fix commits and
+wait for it again — a re-review-per-fix loop burns ~10 min a pass and drags a
+small PR out for an hour (Steve's explicit call, 2026-07-22). The initial review
+is the check; your own build/test/judgment covers the fixes. Gadfly is advisory
+and never blocks merge, so green build + fixes applied is enough.

-**A skipped Gadfly run reports success.** A comment without the trigger phrase
-still starts the workflow, which logs `comment does not contain trigger phrase`
-and exits green in ~2 seconds. So "the pipeline is green" does NOT mean "this
-was reviewed". Confirm a re-review actually ran by its **duration** — a real
-pass takes ~10 minutes, a skip takes 2 seconds. Don't look for a new consensus
-comment: Gadfly EDITS its existing status-board and consensus comments in place,
-so their `created_at` stays at the first review and only `updated_at` moves.
+(Mechanics, if you ever *do* need a manual re-run: the workflow triggers on
+`opened`/`reopened`/`ready_for_review`, not `synchronize`, so pushes don't
+re-review; a `@gadfly review` comment does. A comment without that exact phrase
+still runs and exits green in ~2s — a skip that looks like a pass, so judge a
+real review by its ~10-min duration, not its status. Gadfly edits its consensus
+comment in place, so `updated_at` moves but `created_at` doesn't.)

 Workflow- and config-only changes (CI, this file, docs) go straight to `main`
 without the PR dance.
@@ -65,6 +65,8 @@ POST /change-sets/:id/revert  ← undo an operation; 201, or 409 + the conflicts
 POST /gardens/:id/copy        ← deep-copy a garden you own (objects + active plops; not shares/link)
 POST /gardens/:id/objects     PATCH,DELETE /objects/:id
 POST /objects/:id/plantings   PATCH,DELETE /plantings/:id
+POST /objects/:id/fill        ← hex-pack a region with one plant; region by compass name or rect
+POST /objects/:id/clear       ← soft-remove every active plop, as ONE change set
 GET,POST /plants              PATCH,DELETE /plants/:id            (own plants only)
 GET,POST /seed-lots           GET,PATCH,DELETE /seed-lots/:id     (own lots only; private)
 GET,POST /gardens/:id/journal PATCH,DELETE /journal/:id           (editor writes; author edits own)
@@ -74,6 +76,8 @@ GET,DELETE /gardens/:id/agent/history                             (the actor's o
 GET  /capabilities            ← what this instance can do RIGHT NOW (tracks the live agent, not just config)
 GET,PATCH /settings           ← instance-wide config (admin only): agent model + on/off
 GET,POST /gardens/:id/shares  PATCH,DELETE /gardens/:id/shares/:userId    (invite by email)
+GET,POST,DELETE /gardens/:id/share-link   ← the public read-only token for this garden
+GET  /public/gardens/:token   ← UNAUTHENTICATED read-only /full; the token is the capability
 ```

 **Sync:** plain REST + optimistic UI + last-write-wins with a version guard. Every PATCH/DELETE carries the row's `version`; the server increments on write and returns **409 + the current row** on mismatch; the client rolls back and refetches. No websockets/CRDT — the right cost for household-scale co-editing. Drags PATCH once on drop, not per frame.
@@ -118,6 +118,21 @@ func (h *handlers) agentChat(c *gin.Context) {
 	send(chatEvent{Done: turn})
 }

+// sseWriteTimeout bounds ONE write to the stream, not the stream itself.
+//
+// It is refreshed per frame, which is the only shape that satisfies both ends:
+// the server's absolute WriteTimeout would cut a long turn (#78), while removing
+// the deadline entirely would let a client that stops reading block a write
+// forever once the socket buffer fills — pinning the run goroutine and this
+// stream's mutex with it, and taking the keep-alive down too since it needs the
+// same lock. Generous, because it is a backstop against a stuck peer and not a
+// pacing mechanism.
+//
+// A var, not a const, ONLY so the test can shrink it to prove the deadline is
+// refreshed per frame rather than set once — a set-once 30s deadline would pass
+// a test whose whole run is under a second. Production never reassigns it.
+var sseWriteTimeout = 30 * time.Second
+
 // eventStream serializes writes to one SSE response.
 //
 // The mutex is load-bearing, not decoration: step events are sent from the
@@ -126,6 +141,7 @@ func (h *handlers) agentChat(c *gin.Context) {
 // frames long before it crashes anything.
 type eventStream struct {
 	c  *gin.Context
+	rc *http.ResponseController
 	mu sync.Mutex
 }

@@ -134,12 +150,34 @@ type eventStream struct {
 // Headers go out before the first write and the stream is flushed immediately,
 // so a proxy holding the response until it looks complete can't reintroduce
 // exactly the silence streaming exists to remove.
+//
+// Taking the write deadline off the server's absolute WriteTimeout and onto a
+// per-write one is what makes a turn longer than 30s possible at all (#78).
+// WriteTimeout is an ABSOLUTE deadline from when the request header was read,
+// not an idle timeout, so a streaming response is cut mid-turn however recently
+// it wrote. Without this the 4-minute runTimeout is unreachable and the
+// keep-alive below tops out at one tick — pacing a connection that is destroyed
+// underneath it.
+//
+// That failure is INVISIBLE from in here: writes past the deadline return
+// err == nil and their bytes are dropped, so there is nothing to detect on the
+// write path. Only the client sees it, as a truncated stream it reports as a
+// dropped connection. Hence a deadline set up front and refreshed per frame,
+// rather than anything checked after the fact.
 func openEventStream(c *gin.Context) *eventStream {
 	c.Header("Content-Type", "text/event-stream")
 	c.Header("Cache-Control", "no-cache")
 	c.Header("X-Accel-Buffering", "no")
+	s := &eventStream{c: c, rc: http.NewResponseController(c.Writer)}
+	// Probe once here rather than reporting per frame: a writer that can't take
+	// deadlines will fail identically on every write, and the operator needs to
+	// hear it once. If this fails the stream still works — it is just back to
+	// being cut at WriteTimeout, which is worth saying out loud.
+	if err := s.rc.SetWriteDeadline(time.Now().Add(sseWriteTimeout)); err != nil {
+		slog.Error("api: SSE write deadlines unavailable; long turns will be truncated at the server WriteTimeout", "error", err)
+	}
 	c.Writer.Flush()
-	return &eventStream{c: c}
+	return s
 }

 func (s *eventStream) send(ev chatEvent) {
@@ -154,6 +192,11 @@ func (s *eventStream) send(ev chatEvent) {
 func (s *eventStream) write(frame string) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
+	// Refresh for THIS write, so the stream as a whole is unbounded but no single
+	// write is. Error deliberately unchecked: openEventStream already reported
+	// whether deadlines work at all, and this call can only fail the same way, so
+	// checking here would log once per frame to say the same thing.
+	_ = s.rc.SetWriteDeadline(time.Now().Add(sseWriteTimeout))
 	_, _ = io.WriteString(s.c.Writer, frame)
 	s.c.Writer.Flush()
 }
@@ -122,6 +122,11 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
 	objects.PATCH("/:id", h.updateObject)
 	objects.DELETE("/:id", h.deleteObject)
 	objects.POST("/:id/plantings", h.createPlanting) // place a plop in this object
+	// Bulk ops. These wrap the same service methods the agent tools call, so an
+	// instance with no model configured still gets the most valuable operation in
+	// the app — and so "clear bed" is ONE change set rather than one per plop.
+	objects.POST("/:id/fill", h.fillObject)
+	objects.POST("/:id/clear", h.clearObject)

 	// Plantings ("plops") are addressed by their own id; the service resolves the
 	// owning object/garden for the permission check.
@@ -0,0 +1,116 @@
+package api
+
+import (
+	"net/http"
+
+	"github.com/gin-gonic/gin"
+
+	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
+	"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
+)
+
+// Bulk operations on a plantable object (#82): fill a region with one plant, and
+// clear everything out of it.
+//
+// These were reachable only through the agent toolbox until now, which meant the
+// most valuable bulk operation in a garden planner — and the one carrying the
+// most carefully reasoned geometry in the codebase — did not exist at all on an
+// instance with no model configured. They are thin adapters over the same
+// service methods `internal/agent/tools.go` calls, so the permission checks and
+// the one-change-set-per-operation guarantee come along unchanged.
+
+// fillRect is an explicit rectangle in the object's local frame, the alternative
+// to a compass name. A named type (not an inline anonymous struct) to match the
+// rest of internal/api and so it can carry its own validity check.
+type fillRect struct {
+	MinX float64 `json:"minXCm"`
+	MinY float64 `json:"minYCm"`
+	MaxX float64 `json:"maxXCm"`
+	MaxY float64 `json:"maxYCm"`
+}
+
+// degenerate reports whether the rect encloses no area. Such a rect (including
+// the all-zeros an empty `"rect": {}` decodes to) would otherwise slip through
+// and plant a single plop at the object's centre — a surprising result for what
+// is really malformed input.
+func (r fillRect) degenerate() bool {
+	return r.MaxX <= r.MinX || r.MaxY <= r.MinY
+}
+
+// objectFillRequest is the body for POST /objects/:id/fill.
+//
+// A region is given EITHER by compass name ("ne", "south half", "all") or as an
+// explicit rect in the object's local frame. The named form is what a person
+// means and what the agent uses; the rect is for a future drag-a-box affordance.
+// Exactly one must be supplied — accepting both and silently preferring one
+// would make a client bug look like a geometry bug.
+type objectFillRequest struct {
+	PlantID int64     `json:"plantId" binding:"required"`
+	Region  string    `json:"region"`
+	Rect    *fillRect `json:"rect"`
+	// SpacingOverrideCM plants tighter or looser than the plant's mature spacing
+	// without editing the catalog entry.
+	SpacingOverrideCM *float64 `json:"spacingOverrideCm"`
+}
+
+func (h *handlers) fillObject(c *gin.Context) {
+	id, ok := parseIDParam(c, "id")
+	if !ok {
+		return
+	}
+	var req objectFillRequest
+	if err := c.ShouldBindJSON(&req); err != nil {
+		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a plantId and a region are required")
+		return
+	}
+	named, hasRect := req.Region != "", req.Rect != nil
+	if named == hasRect {
+		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT",
+			`supply exactly one of "region" (e.g. "all", "ne", "south half") or "rect"`)
+		return
+	}
+
+	actor := mustActor(c).ID
+	var (
+		created []domain.Planting
+		err     error
+	)
+	if rect := req.Rect; rect != nil {
+		// Reject a zero-area rect here rather than let it plant one stray plop.
+		// (Binding the pointer to `rect` also keeps the deref visibly guarded,
+		// instead of reading req.Rect.MinX under an invariant from a line above.)
+		if rect.degenerate() {
+			writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "rect must enclose a positive area")
+			return
+		}
+		region := service.Region{MinX: rect.MinX, MinY: rect.MinY, MaxX: rect.MaxX, MaxY: rect.MaxY}
+		created, err = h.svc.FillRegion(c.Request.Context(), actor, id, region, req.PlantID, req.SpacingOverrideCM)
+	} else {
+		created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM)
+	}
+	if err != nil {
+		writeServiceError(c, err)
+		return
+	}
+	// 200, not 201: a fill can legitimately create nothing (the region is already
+	// planted), and there is no single resource to point a Location at.
+	c.JSON(http.StatusOK, gin.H{"plantings": created, "created": len(created)})
+}
+
+// clearObject soft-removes every active plop in an object.
+//
+// Distinct from deleting the object, and — unlike the client-side loop this
+// replaces — it lands as ONE change set, so undoing a cleared bed is one click
+// rather than one per plop.
+func (h *handlers) clearObject(c *gin.Context) {
+	id, ok := parseIDParam(c, "id")
+	if !ok {
+		return
+	}
+	n, err := h.svc.ClearObject(c.Request.Context(), mustActor(c).ID, id)
+	if err != nil {
+		writeServiceError(c, err)
+		return
+	}
+	c.JSON(http.StatusOK, gin.H{"cleared": n})
+}
@@ -0,0 +1,226 @@
+package api
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+)
+
+func fillPath(id int64) string  { return objectPath(id) + "/fill" }
+func clearPath(id int64) string { return objectPath(id) + "/clear" }
+
+// makeFillPlant creates a custom plant and returns its id. (A near-identical
+// createPlantAPI landed alongside the seed-lot tests; consolidating the two into
+// one shared helper is a fine follow-up, kept separate here only to avoid a
+// merge collision on the shared symbol.)
+func makeFillPlant(t *testing.T, r *gin.Engine, cookie *http.Cookie, name string, spacing float64) int64 {
+	t.Helper()
+	w := doJSON(t, r, http.MethodPost, "/api/v1/plants", map[string]any{
+		"name": name, "category": "vegetable", "spacingCm": spacing, "color": "#4a7c3f", "icon": "🌱",
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create plant %q: status %d, body %s", name, w.Code, w.Body.String())
+	}
+	return int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+}
+
+// seedFillableBed makes a garden with one plantable bed and a custom plant,
+// returning (gardenID, objectID, plantID).
+func seedFillableBed(t *testing.T, r *gin.Engine, cookie *http.Cookie, w, h, spacing float64) (int64, int64, int64) {
+	t.Helper()
+	gid := createGardenAPI(t, r, cookie, "G")
+	rec := doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{
+		"kind": "bed", "widthCm": w, "heightCm": h, "plantable": true,
+	}, cookie)
+	if rec.Code != http.StatusCreated {
+		t.Fatalf("create bed: status %d, body %s", rec.Code, rec.Body.String())
+	}
+	objID := int64(decodeMap(t, rec.Body.Bytes())["id"].(float64))
+	plantID := makeFillPlant(t, r, cookie, "Fillable", spacing)
+	return gid, objID, plantID
+}
+
+// countChangeSets reads the history page and reports how many change sets exist.
+func countChangeSets(t *testing.T, r *gin.Engine, cookie *http.Cookie, gardenID int64) int {
+	t.Helper()
+	w := doJSON(t, r, http.MethodGet, historyPath(gardenID), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("history: status %d, body %s", w.Code, w.Body.String())
+	}
+	sets, _ := decodeMap(t, w.Body.Bytes())["changeSets"].([]any)
+	return len(sets)
+}
+
+// TestFillAndClearAPI covers the two routes end to end through the router.
+//
+// These exist because both operations were previously reachable ONLY through the
+// agent toolbox, so on an instance with no model configured the most valuable
+// bulk operation in the app did not exist at all.
+func TestFillAndClearAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	_, objID, plantID := seedFillableBed(t, r, cookie, 200, 200, 20)
+
+	// Fill by compass name.
+	w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
+		"plantId": plantID, "region": "all",
+	}, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("fill: status %d, body %s", w.Code, w.Body.String())
+	}
+	body := decodeMap(t, w.Body.Bytes())
+	created := int(body["created"].(float64))
+	if created == 0 {
+		t.Fatalf("fill created nothing: %s", w.Body.String())
+	}
+	if plops, _ := body["plantings"].([]any); len(plops) != created {
+		t.Errorf("created=%d but returned %d plantings", created, len(plops))
+	}
+
+	// Clear it: one call, and it reports what it removed.
+	w = doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("clear: status %d, body %s", w.Code, w.Body.String())
+	}
+	if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != created {
+		t.Errorf("cleared %d, want %d (everything the fill made)", n, created)
+	}
+
+	// Clearing an already-empty bed is a no-op, not an error.
+	w = doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("second clear: status %d", w.Code)
+	}
+	if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != 0 {
+		t.Errorf("second clear removed %d, want 0", n)
+	}
+}
+
+// TestFillRegionSelectionAPI: exactly one of region/rect, and a rect fills only
+// its own corner of the bed.
+func TestFillRegionSelectionAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	_, objID, plantID := seedFillableBed(t, r, cookie, 400, 400, 20)
+
+	// Neither → 400. Both → 400. Accepting both and silently preferring one
+	// would make a client bug look like a geometry bug.
+	if w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{"plantId": plantID}, cookie); w.Code != http.StatusBadRequest {
+		t.Errorf("no region = %d, want 400", w.Code)
+	}
+	both := map[string]any{
+		"plantId": plantID, "region": "all",
+		"rect": map[string]any{"minXCm": -50, "minYCm": -50, "maxXCm": 50, "maxYCm": 50},
+	}
+	if w := doJSON(t, r, http.MethodPost, fillPath(objID), both, cookie); w.Code != http.StatusBadRequest {
+		t.Errorf("both region and rect = %d, want 400", w.Code)
+	}
+	// An unknown compass name is rejected rather than silently filling nothing.
+	if w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
+		"plantId": plantID, "region": "middle-ish",
+	}, cookie); w.Code != http.StatusBadRequest {
+		t.Errorf("bad region name = %d, want 400", w.Code)
+	}
+
+	// A zero-area rect is malformed input, not "plant one at the centre". An empty
+	// `"rect": {}` decodes to all-zeros and must be caught the same way.
+	for _, rect := range []map[string]any{
+		{}, // {} → 0,0,0,0
+		{"minXCm": 10, "minYCm": 10, "maxXCm": 10, "maxYCm": 50}, // zero width
+		{"minXCm": 10, "minYCm": 50, "maxXCm": 50, "maxYCm": 50}, // zero height
+		{"minXCm": 50, "minYCm": 50, "maxXCm": 10, "maxYCm": 10}, // inverted
+	} {
+		if w := doJSON(t, r, http.MethodPost, fillPath(objID),
+			map[string]any{"plantId": plantID, "rect": rect}, cookie); w.Code != http.StatusBadRequest {
+			t.Errorf("degenerate rect %v = %d, want 400", rect, w.Code)
+		}
+	}
+
+	// A rect confined to the NE corner produces plops only there. Local frame:
+	// +x east, -y north.
+	w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
+		"plantId": plantID,
+		"rect":    map[string]any{"minXCm": 0, "minYCm": -200, "maxXCm": 200, "maxYCm": 0},
+	}, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("rect fill: status %d, body %s", w.Code, w.Body.String())
+	}
+	plops, _ := decodeMap(t, w.Body.Bytes())["plantings"].([]any)
+	if len(plops) == 0 {
+		t.Fatal("rect fill created nothing")
+	}
+	for _, raw := range plops {
+		p := raw.(map[string]any)
+		if x, y := p["xCm"].(float64), p["yCm"].(float64); x < 0 || y > 0 {
+			t.Errorf("plop at (%v,%v) outside the NE rect", x, y)
+		}
+	}
+}
+
+// TestClearObjectIsOneChangeSetAPI is the regression test for the behaviour this
+// endpoint exists to restore.
+//
+// The UI used to clear a bed with a loop of PATCHes, and since every service
+// mutation auto-scopes its own change set, clearing a 40-plop bed wrote 40 of
+// them — 40 presses of Undo to put the bed back. CLAUDE.md states the rule
+// directly: multi-row operations record together so they undo as one unit.
+func TestClearObjectIsOneChangeSetAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	gid, objID, plantID := seedFillableBed(t, r, cookie, 300, 300, 20)
+
+	w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
+		"plantId": plantID, "region": "all",
+	}, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("fill: status %d, body %s", w.Code, w.Body.String())
+	}
+	created := int(decodeMap(t, w.Body.Bytes())["created"].(float64))
+	if created < 4 {
+		t.Fatalf("need several plops to make this meaningful, got %d", created)
+	}
+
+	before := countChangeSets(t, r, cookie, gid)
+	if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie); w.Code != http.StatusOK {
+		t.Fatalf("clear: status %d", w.Code)
+	}
+	if after := countChangeSets(t, r, cookie, gid); after != before+1 {
+		t.Errorf("clearing %d plops added %d change sets, want exactly 1", created, after-before)
+	}
+}
+
+// TestFillClearPermissionsAPI: a viewer may look but not fill or clear, and a
+// stranger gets 404 because existence is masked.
+func TestFillClearPermissionsAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	owner := registerAndCookie(t, r, "[email protected]")
+	viewer := registerAndCookie(t, r, "[email protected]")
+	stranger := registerAndCookie(t, r, "[email protected]")
+
+	gid, objID, plantID := seedFillableBed(t, r, owner, 200, 200, 20)
+	if w := doJSON(t, r, http.MethodPost, sharesPath(gid),
+		map[string]any{"email": "[email protected]", "role": "viewer"}, owner); w.Code != http.StatusCreated {
+		t.Fatalf("share as viewer: status %d, body %s", w.Code, w.Body.String())
+	}
+
+	fillBody := map[string]any{"plantId": plantID, "region": "all"}
+	if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, viewer); w.Code != http.StatusForbidden {
+		t.Errorf("viewer fill = %d, want 403 (they can see it but may not do that)", w.Code)
+	}
+	if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, viewer); w.Code != http.StatusForbidden {
+		t.Errorf("viewer clear = %d, want 403", w.Code)
+	}
+	if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, stranger); w.Code != http.StatusNotFound {
+		t.Errorf("stranger fill = %d, want 404 (existence masked)", w.Code)
+	}
+	if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, stranger); w.Code != http.StatusNotFound {
+		t.Errorf("stranger clear = %d, want 404", w.Code)
+	}
+	if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, nil); w.Code != http.StatusUnauthorized {
+		t.Errorf("anonymous fill = %d, want 401", w.Code)
+	}
+	if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, nil); w.Code != http.StatusUnauthorized {
+		t.Errorf("anonymous clear = %d, want 401", w.Code)
+	}
+}
@@ -0,0 +1,264 @@
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"strconv"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+)
+
+func seedLotPath(id int64) string {
+	return "/api/v1/seed-lots/" + strconv.FormatInt(id, 10)
+}
+
+// decodeList decodes a bare JSON array body. Seed lot listing returns the array
+// directly rather than wrapping it (unlike /journal's {"entries": …}), so a
+// helper that assumed an object would quietly read nothing.
+func decodeList(t *testing.T, body []byte) []any {
+	t.Helper()
+	var out []any
+	if err := json.Unmarshal(body, &out); err != nil {
+		t.Fatalf("decode list: %v (%s)", err, body)
+	}
+	return out
+}
+
+// createPlantAPI makes a custom plant and returns its id.
+func createPlantAPI(t *testing.T, r *gin.Engine, cookie *http.Cookie, name string, spacing float64) int64 {
+	t.Helper()
+	w := doJSON(t, r, http.MethodPost, "/api/v1/plants", map[string]any{
+		"name": name, "category": "vegetable", "spacingCm": spacing, "color": "#4a7c3f", "icon": "🌱",
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create plant %q: status %d, body %s", name, w.Code, w.Body.String())
+	}
+	return int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+}
+
+// TestSeedLotCrudAPI walks the whole seed lot lifecycle over HTTP.
+//
+// It exists for the reason CLAUDE.md gives: service tests cannot see a route
+// that was never registered, or one registered with the wrong :param name. Every
+// other handler file had a sibling API test; this group did not, which is the
+// state PATCH/DELETE /journal/:id shipped in — implemented, unit-tested, and
+// completely unreachable.
+func TestSeedLotCrudAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	plantID := createPlantAPI(t, r, cookie, "Music Garlic", 15)
+
+	// Create.
+	w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{
+		"plantId": plantID, "vendor": "Johnny's", "sku": "2761",
+		"quantity": 100, "unit": "seeds", "packedForYear": 2026, "costCents": 495,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create: status %d, body %s", w.Code, w.Body.String())
+	}
+	lot := decodeMap(t, w.Body.Bytes())
+	id := int64(lot["id"].(float64))
+	if lot["vendor"] != "Johnny's" || lot["unit"] != "seeds" {
+		t.Errorf("unexpected lot: %+v", lot)
+	}
+
+	// GET by id — the route most likely to be missing or mis-registered.
+	w = doJSON(t, r, http.MethodGet, seedLotPath(id), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("get: status %d, body %s", w.Code, w.Body.String())
+	}
+	if got := decodeMap(t, w.Body.Bytes()); int64(got["id"].(float64)) != id {
+		t.Errorf("get returned id %v, want %d", got["id"], id)
+	}
+
+	// List, and the ?plantId= filter.
+	w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots", nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("list: status %d, body %s", w.Code, w.Body.String())
+	}
+	if n := len(decodeList(t, w.Body.Bytes())); n != 1 {
+		t.Fatalf("list returned %d lots, want 1: %s", n, w.Body.String())
+	}
+
+	other := createPlantAPI(t, r, cookie, "Cherokee Purple", 45)
+	w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId="+strconv.FormatInt(other, 10), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("filtered list: status %d, body %s", w.Code, w.Body.String())
+	}
+	if n := len(decodeList(t, w.Body.Bytes())); n != 0 {
+		t.Errorf("filter by a plant with no lots returned %d, want 0", n)
+	}
+	// A bad plantId filter is 400, whether non-numeric or out of range — the
+	// handler rejects id < 1, not just unparseable strings.
+	for _, bad := range []string{"nope", "0", "-1"} {
+		if w := doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId="+bad, nil, cookie); w.Code != http.StatusBadRequest {
+			t.Errorf("plantId=%q filter = %d, want 400", bad, w.Code)
+		}
+	}
+
+	// PATCH with the current version.
+	w = doJSON(t, r, http.MethodPatch, seedLotPath(id), map[string]any{
+		"vendor": "Fedco", "version": lot["version"],
+	}, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("patch: status %d, body %s", w.Code, w.Body.String())
+	}
+	updated := decodeMap(t, w.Body.Bytes())
+	if updated["vendor"] != "Fedco" {
+		t.Errorf("vendor = %v, want Fedco", updated["vendor"])
+	}
+	if updated["version"].(float64) != lot["version"].(float64)+1 {
+		t.Errorf("patch didn't bump version: %v -> %v", lot["version"], updated["version"])
+	}
+
+	// A stale version conflicts and carries the current row back, so the client
+	// can rebase without a second request.
+	w = doJSON(t, r, http.MethodPatch, seedLotPath(id), map[string]any{
+		"vendor": "stale", "version": lot["version"],
+	}, cookie)
+	if w.Code != http.StatusConflict {
+		t.Fatalf("stale patch: status %d, want 409", w.Code)
+	}
+	if cur, ok := decodeMap(t, w.Body.Bytes())["current"].(map[string]any); !ok || cur["vendor"] != "Fedco" {
+		t.Errorf("409 body missing the current row: %s", w.Body.String())
+	}
+
+	// DELETE.
+	if w := doJSON(t, r, http.MethodDelete, seedLotPath(id), nil, cookie); w.Code != http.StatusNoContent {
+		t.Fatalf("delete: status %d, body %s", w.Code, w.Body.String())
+	}
+	if w := doJSON(t, r, http.MethodGet, seedLotPath(id), nil, cookie); w.Code != http.StatusNotFound {
+		t.Errorf("get after delete = %d, want 404", w.Code)
+	}
+}
+
+// TestSeedLotRemainingIsDerivedAPI checks that `remaining` reflects plantings
+// through the HTTP surface, not just in the service.
+//
+// DESIGN.md makes derivation load-bearing — "a decremented column drifts the
+// moment a planting is edited behind its back" — so a route that returned a
+// stored or stale figure would break the invariant silently, and the number is
+// the whole reason anyone opens the seed shelf.
+func TestSeedLotRemainingIsDerivedAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	plantID := createPlantAPI(t, r, cookie, "Beans", 10)
+
+	w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{
+		"plantId": plantID, "quantity": 50, "unit": "seeds",
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create lot: status %d, body %s", w.Code, w.Body.String())
+	}
+	lotID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+
+	gid := createGardenAPI(t, r, cookie, "G")
+	w = doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{
+		"kind": "bed", "widthCm": 100, "heightCm": 100, "plantable": true,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create object: status %d, body %s", w.Code, w.Body.String())
+	}
+	oid := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+
+	// Plant 12 of them against the lot.
+	w = doJSON(t, r, http.MethodPost, objectPlantingsPath(oid), map[string]any{
+		"plantId": plantID, "xCm": 0, "yCm": 0, "radiusCm": 20, "count": 12, "seedLotId": lotID,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create planting: status %d, body %s", w.Code, w.Body.String())
+	}
+
+	w = doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("get lot: status %d, body %s", w.Code, w.Body.String())
+	}
+	got := decodeMap(t, w.Body.Bytes())
+	if rem, ok := got["remaining"].(float64); !ok || rem != 38 {
+		t.Errorf("remaining = %v, want 38 (50 bought - 12 planted): %s", got["remaining"], w.Body.String())
+	}
+
+	// Over-plant it: 45 more (57 total against 50 bought) drives remaining
+	// NEGATIVE. That's deliberate — the number is a derived truth about what
+	// you've committed, not a floor clamped at zero, and "you've planted more than
+	// you bought" is exactly the signal a gardener wants rather than a hidden -7.
+	w = doJSON(t, r, http.MethodPost, objectPlantingsPath(oid), map[string]any{
+		"plantId": plantID, "xCm": 40, "yCm": 40, "radiusCm": 20, "count": 45, "seedLotId": lotID,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("over-plant: status %d, body %s", w.Code, w.Body.String())
+	}
+	w = doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, cookie)
+	if rem, ok := decodeMap(t, w.Body.Bytes())["remaining"].(float64); !ok || rem != -7 {
+		t.Errorf("remaining after over-planting = %v, want -7 (50 - 57)", rem)
+	}
+}
+
+// TestSeedLotsArePrivateAPI checks the ACL through the router.
+//
+// Lots are private to the buyer and deliberately never travel with a shared
+// garden, so another user must not be able to read or edit one. Per the
+// project's convention, no-access is ErrNotFound rather than ErrForbidden —
+// existence is masked — so every one of these is a 404, not a 403.
+func TestSeedLotsArePrivateAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	alice := registerAndCookie(t, r, "[email protected]")
+	bob := registerAndCookie(t, r, "[email protected]")
+
+	plantID := createPlantAPI(t, r, alice, "Alice's garlic", 15)
+	w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{
+		"plantId": plantID, "vendor": "Secret Vendor", "quantity": 10, "unit": "bulbs",
+	}, alice)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("alice create: status %d, body %s", w.Code, w.Body.String())
+	}
+	lotID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+
+	for _, tc := range []struct {
+		name   string
+		method string
+		body   any
+	}{
+		{"get", http.MethodGet, nil},
+		{"patch", http.MethodPatch, map[string]any{"vendor": "hijacked", "version": 1}},
+		{"delete", http.MethodDelete, nil},
+	} {
+		if w := doJSON(t, r, tc.method, seedLotPath(lotID), tc.body, bob); w.Code != http.StatusNotFound {
+			t.Errorf("bob %s = %d, want 404 (existence is masked)", tc.name, w.Code)
+		}
+	}
+
+	// Bob's own listing must not include it either.
+	w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots", nil, bob)
+	if w.Code != http.StatusOK {
+		t.Fatalf("bob list: status %d", w.Code)
+	}
+	if n := len(decodeList(t, w.Body.Bytes())); n != 0 {
+		t.Errorf("bob sees %d of alice's lots, want 0: %s", n, w.Body.String())
+	}
+
+	// And it's still intact for alice.
+	if w := doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, alice); w.Code != http.StatusOK {
+		t.Errorf("alice lost access to her own lot: %d", w.Code)
+	}
+}
+
+// TestSeedLotsRequireAuthAPI: the group is behind requireAuth, and an
+// unauthenticated caller gets 401 rather than an empty list.
+func TestSeedLotsRequireAuthAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	for _, tc := range []struct {
+		method, path string
+	}{
+		{http.MethodGet, "/api/v1/seed-lots"},
+		{http.MethodPost, "/api/v1/seed-lots"},
+		{http.MethodGet, seedLotPath(1)},
+		{http.MethodPatch, seedLotPath(1)},
+		{http.MethodDelete, seedLotPath(1)},
+	} {
+		if w := doJSON(t, r, tc.method, tc.path, nil, nil); w.Code != http.StatusUnauthorized {
+			t.Errorf("%s %s = %d, want 401", tc.method, tc.path, w.Code)
+		}
+	}
+}
@@ -0,0 +1,101 @@
+package api
+
+import (
+	"bufio"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/gin-gonic/gin"
+)
+
+// streamFrames spins up a real http.Server with the given WriteTimeout and an
+// SSE handler that emits `frames` data frames, one every `tick`, then returns.
+// It reports how many frames the client actually received and any read error —
+// the only vantage point from which the deadline failures in #78/#87 are
+// visible, since the writes themselves return nil when the bytes are dropped.
+func streamFrames(t *testing.T, serverWriteTimeout, tick time.Duration, frames int) (int, error) {
+	t.Helper()
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	r.GET("/stream", func(c *gin.Context) {
+		s := openEventStream(c)
+		for i := 0; i < frames; i++ {
+			time.Sleep(tick)
+			s.send(chatEvent{Error: "frame"})
+		}
+	})
+
+	srv := httptest.NewUnstartedServer(r)
+	srv.Config.WriteTimeout = serverWriteTimeout
+	srv.Start()
+	defer srv.Close()
+
+	resp, err := srv.Client().Get(srv.URL + "/stream")
+	if err != nil {
+		t.Fatalf("get: %v", err)
+	}
+	defer resp.Body.Close()
+
+	got := 0
+	sc := bufio.NewScanner(resp.Body)
+	for sc.Scan() {
+		if strings.HasPrefix(sc.Text(), "data: ") {
+			got++
+		}
+	}
+	return got, sc.Err()
+}
+
+// TestEventStreamOutlivesServerWriteTimeout is the regression test for #78.
+//
+// http.Server.WriteTimeout is an ABSOLUTE deadline measured from when the
+// request header was read — not an idle timeout — so a streaming response is cut
+// once it passes, however recently the handler wrote. pansy sets it to 30s while
+// an agent turn may run for minutes. openEventStream must override it.
+//
+// This has to be asserted from the CLIENT side because the failure cannot be
+// observed from the handler: writes made after the deadline return err == nil
+// and their bytes are silently discarded. A test that checked the return of
+// io.WriteString would pass against the bug.
+func TestEventStreamOutlivesServerWriteTimeout(t *testing.T) {
+	// sseWriteTimeout stays at its 30s default here, so the per-frame refresh
+	// keeps the stream alive with a huge margin — CI slowness only ever makes
+	// this pass more surely. The server's 300ms WriteTimeout is the thing being
+	// overridden; frames straddle it (300ms/600ms/900ms).
+	got, err := streamFrames(t, 300*time.Millisecond, 300*time.Millisecond, 3)
+	if err != nil {
+		t.Errorf("client read error after %d/3 frames: %v", got, err)
+	}
+	if got != 3 {
+		t.Errorf("client received %d frames, want 3 — the stream was cut at the server WriteTimeout", got)
+	}
+}
+
+// TestEventStreamRefreshesDeadlinePerFrame guards the #87 fix specifically: the
+// write deadline is refreshed on EVERY frame, not set once.
+//
+// A set-once deadline is a plausible "simplification" and it reintroduces the
+// unbounded-block risk the per-frame refresh exists to prevent — yet it would
+// sail through the test above, whose whole run is far under sseWriteTimeout. So
+// shrink sseWriteTimeout below the stream's total duration and send frames whose
+// gap stays comfortably under it: per-frame refresh delivers them all, while a
+// deadline set once at open would expire mid-stream and cut it short.
+func TestEventStreamRefreshesDeadlinePerFrame(t *testing.T) {
+	orig := sseWriteTimeout
+	sseWriteTimeout = 400 * time.Millisecond
+	t.Cleanup(func() { sseWriteTimeout = orig })
+
+	// The server WriteTimeout is generous (5s), so it isn't the limiter — the
+	// per-frame sseWriteTimeout is. 8 frames at a 100ms tick span 800ms, well past
+	// the 400ms deadline, but each 100ms gap is a 4× margin under it.
+	got, err := streamFrames(t, 5*time.Second, 100*time.Millisecond, 8)
+	if err != nil {
+		t.Errorf("client read error after %d/8 frames: %v", got, err)
+	}
+	if got != 8 {
+		t.Errorf("client received %d frames, want 8 — a set-once deadline would cut the stream at ~%v; the refresh must be per-frame",
+			got, sseWriteTimeout)
+	}
+}
@@ -1,5 +1,12 @@
 import { useEffect, useRef, type ReactNode } from 'react'

+// Tabbable controls inside the dialog, in DOM order. type="hidden" inputs are
+// excluded — they'd match `input:not([disabled])` and, sitting at a boundary,
+// break the wrap math. Hoisted out of the handler so it isn't rebuilt per Tab.
+const FOCUSABLE_SELECTOR =
+  'a[href], button:not([disabled]), textarea:not([disabled]), ' +
+  'input:not([disabled]):not([type="hidden"]), select:not([disabled]), [tabindex]:not([tabindex="-1"])'
+
 /**
 * A centered modal dialog over a dimmed backdrop. Closes on Escape or a backdrop
 * click, unless `busy` (a mutation is in flight) — then it stays put so the
@@ -27,12 +34,57 @@ export function Modal({
  busyRef.current = busy

  useEffect(() => {
-    cardRef.current?.focus()
+    const card = cardRef.current
+    // Remember who opened the dialog so focus can return there on close —
+    // otherwise it lands on <body> and a keyboard user loses their place.
+    const opener = document.activeElement as HTMLElement | null
+    card?.focus()
+
+    const focusable = () =>
+      Array.from(card?.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR) ?? [])
+
    function onKey(e: KeyboardEvent) {
-      if (e.key === 'Escape' && !busyRef.current) onCloseRef.current()
+      if (e.key === 'Escape' && !busyRef.current) {
+        onCloseRef.current()
+        return
+      }
+      if (e.key !== 'Tab') return
+      const items = focusable()
+      if (items.length === 0) {
+        e.preventDefault()
+        card?.focus()
+        return
+      }
+      const first = items[0]
+      const last = items[items.length - 1]
+      const active = document.activeElement
+      // If focus is NOT inside the dialog, pull it back in rather than let Tab
+      // escape. This is the robust case that covers focus having fallen to
+      // <body> — a control that was removed (ShareGardenModal's remove-share
+      // button) or disabled while busy — as well as any externally-stolen focus.
+      if (!card || !card.contains(active)) {
+        e.preventDefault()
+        ;(e.shiftKey ? last : first).focus()
+        return
+      }
+      if (e.shiftKey && (active === first || active === card)) {
+        e.preventDefault()
+        last.focus()
+      } else if (!e.shiftKey && active === last) {
+        e.preventDefault()
+        first.focus()
+      }
    }
    document.addEventListener('keydown', onKey)
-    return () => document.removeEventListener('keydown', onKey)
+    return () => {
+      document.removeEventListener('keydown', onKey)
+      // Restore focus to the opener only if it's still in the document — the
+      // delete/clear flows this trap targets often remove the element that
+      // opened the dialog (a garden card, a plop row). A disconnected node's
+      // focus() silently no-ops and leaves focus on <body>, so fall through to
+      // that case explicitly rather than pretend it worked.
+      if (opener && opener.isConnected) opener.focus()
+    }
  }, [])

  return (
@@ -5,23 +5,25 @@ import { useClearObject } from '@/lib/objects'
 /** Confirm clearing every active plop from a focused bed (soft-remove — the rows
 *  are kept with removed_at, so history survives). */
 export function ClearBedModal({
+  objectId,
  objectName,
-  plops,
+  plopCount,
  gardenId,
  onClose,
 }: {
+  objectId: number
  objectName: string
-  plops: { id: number; version: number }[]
+  plopCount: number
  gardenId: number
  onClose: () => void
 }) {
  const clear = useClearObject(gardenId)
-  const n = plops.length
  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">{n}</span> {n === 1 ? 'plant' : 'plants'} from{' '}
+          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">
@@ -31,8 +33,8 @@ export function ClearBedModal({
          <Button
            type="button"
            variant="danger"
-            disabled={clear.isPending || n === 0}
-            onClick={() => clear.mutate(plops, { onSuccess: onClose })}
+            disabled={clear.isPending || plopCount === 0}
+            onClick={() => clear.mutate(objectId, { onSuccess: onClose })}
          >
            {clear.isPending ? 'Clearing…' : 'Clear bed'}
          </Button>
@@ -235,7 +235,13 @@ export function GardenCanvas({
        className="h-full w-full select-none"
        style={{ touchAction: 'none' }}
        onPointerDown={onCanvasPointerDown}
+        // role="application" tells a screen reader this is an interactive canvas
+        // to operate, not a document to read linearly. The <title> names it, and
+        // objects inside are individually focusable buttons (see ObjectShape).
+        role="application"
+        aria-label={`${garden.name} — garden layout. Tab between objects; Enter selects; arrow keys nudge a selection.`}
      >
+        <title>{garden.name} garden layout</title>
        <g transform={`translate(${viewport.tx} ${viewport.ty}) scale(${viewport.scale})`}>
          {drawnGridCm != null && (
            <>
@@ -1,5 +1,6 @@
-import { memo, type PointerEvent } from 'react'
+import { memo, type KeyboardEvent, type PointerEvent } from 'react'
 import { objectTransform } from './shared'
+import { kindDef, objectDisplayName } from './kinds'
 import type { EditorObject } from './types'

 const DEFAULT_FILL = '#8a8a8a'
@@ -57,11 +58,50 @@ export const ObjectShape = memo(function ObjectShape({
    onSelect(object.id)
  }

+  // Keyboard path into selection (#84): the arrow-key nudge handler already
+  // exists but only ever acted on a pointer selection, so it was unreachable
+  // without a mouse. Enter/Space on a focused object selects it, which is the
+  // step that was missing.
+  function handleKey(e: KeyboardEvent) {
+    if (e.key === 'Enter' || e.key === ' ') {
+      e.preventDefault()
+      e.stopPropagation()
+      onSelect(object.id)
+    }
+  }
+
  const stroke = selected ? '#2f7a3e' : '#00000033'
  const strokeWidth = selected ? 2 : 1

+  // A concise accessible name: the object's label plus its kind's canonical
+  // label, e.g. "North Bed, In-ground" — reusing kindDef so it never diverges
+  // from what the UI shows (an ad-hoc kind.replace() gave "in ground"). The
+  // dimensions aren't included; they need the garden's unit context this
+  // component doesn't hold, so they're a follow-up.
+  const kindLabel = kindDef(object.kind)?.label ?? object.kind
+  const label = `${objectDisplayName(object)}, ${kindLabel}`
+
+  // Keyboard focus needs to be VISIBLE — that's the point of making the canvas
+  // keyboard-reachable. The `object-shape` class carries a :focus-visible rule
+  // (styles/index.css) that draws a dashed ring; :focus-visible means it shows
+  // for keyboard focus but NOT a mouse click, which is exactly what we want. CSS
+  // rather than React state because onFocus on an SVG <g> is unreliable and a
+  // presentation attribute is overridden by any CSS rule.
  return (
-    <g transform={objectTransform(object)} onPointerDown={handleDown} style={{ cursor: 'pointer' }}>
+    <g
+      className="object-shape"
+      transform={objectTransform(object)}
+      onPointerDown={handleDown}
+      onKeyDown={handleKey}
+      role="button"
+      tabIndex={0}
+      aria-label={label}
+      // aria-current, not aria-pressed: selecting an object isn't a toggle (a
+      // toggle is what aria-pressed means). aria-current marks it as the active
+      // item among the objects. Omitted, not "false", when unselected.
+      aria-current={selected || undefined}
+      style={{ cursor: 'pointer' }}
+    >
      {object.shape === 'circle' ? (
        <ellipse
          cx={0}
@@ -328,28 +328,28 @@ export function useUpdatePlanting(gardenId: number) {
  })
 }

-/** Clear a bed: soft-remove every active plop in an object (a loop of PATCHes;
- *  a bulk ClearObject endpoint arrives with the agent seam, #19). Invalidates
- *  once at the end. Pass the object's active plops (id + current version). */
+const clearResultSchema = z.object({ cleared: z.number() })
+
+/** Clear a bed: soft-remove every active plop in an object (#82).
+ *
+ *  ONE request, and so ONE change set. This used to be a loop of PATCHes, which
+ *  meant clearing a 40-plop bed wrote 40 change sets and took 40 presses of Undo
+ *  to put back — while the agent's clear_object, for the identical user-facing
+ *  action, undid in a single click. The rule it violated is stated in CLAUDE.md:
+ *  multi-row operations record all their changes together so they undo as one
+ *  unit. Doing it server-side also removes the partial-failure case the old loop
+ *  had to reconcile. */
 export function useClearObject(gardenId: number) {
  const qc = useQueryClient()
  return useMutation({
-    mutationFn: async (plops: { id: number; version: number }[]) => {
-      const today = new Date().toISOString().slice(0, 10)
-      // allSettled, not all: a partial failure still soft-removed some rows
-      // server-side, so we must reconcile the cache rather than roll everything
-      // back. Report how many failed.
-      const results = await Promise.allSettled(
-        plops.map((p) => api.patch(`/plantings/${p.id}`, { removedAt: today, version: p.version })),
-      )
-      const failed = results.filter((r) => r.status === 'rejected').length
-      if (failed > 0) {
-        throw new Error(`${failed} of ${plops.length} plants couldn't be cleared — refresh and try again.`)
-      }
+    mutationFn: async (objectId: number): Promise<number> => {
+      // No body — clear takes none; passing undefined sends none rather than an
+      // empty {}. The response is just a count; validate it rather than cast.
+      const res = clearResultSchema.parse(await api.post(`/objects/${objectId}/clear`))
+      return res.cleared
    },
-    // Reconcile on success OR partial failure, so the cache matches the server.
    onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),
-    onError: (err) => toast.error(err instanceof Error ? err.message : 'Could not clear the bed.'),
+    onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')),
  })
 }

@@ -526,8 +526,9 @@ export function GardenEditorPage() {

      {clearing && focusedObject && (
        <ClearBedModal
+          objectId={focusedObject.id}
          objectName={objectDisplayName(focusedObject)}
-          plops={focusedPlops.map((p) => ({ id: p.id, version: p.version }))}
+          plopCount={focusedPlops.length}
          gardenId={gid}
          onClose={() => setClearing(false)}
        />
@@ -28,6 +28,19 @@
    font-family: var(--font-sans);
    -webkit-font-smoothing: antialiased;
  }
+
+  /* Keyboard focus on a canvas object (#84). :focus-visible shows the ring for
+     keyboard focus but not a mouse click; the dashed accent ring distinguishes
+     "focused" from the solid ring that marks "selected". A CSS rule overrides
+     the shape's inline stroke presentation attributes. */
+  .object-shape {
+    outline: none;
+  }
+  .object-shape:focus-visible :is(rect, ellipse) {
+    stroke: var(--color-accent-strong);
+    stroke-width: 2;
+    stroke-dasharray: 5 4;
+  }
 }

 /* Dark theme: override the same tokens so utilities recolor automatically. */
Author	SHA1	Message	Date
steveandClaude Opus 4.8	4b348dcbc0	Address Gadfly findings on #84 Build image / build-and-push (push) Successful in 10s Details - Object aria-label uses kindDef().label (the canonical "In-ground") instead of an ad-hoc kind.replace() that produced "in ground" and diverged from the UI. 4+ models flagged this. - aria-current, not aria-pressed, for the selected object — selection isn't a toggle, which is what aria-pressed means; aria-current marks the active item. - Modal focus trap made robust: if focus is NOT inside the dialog (fell to <body> because the focused control was removed — ShareGardenModal's remove-share button — or disabled while busy, or externally stolen), Tab now pulls it back in instead of escaping. The previous branches only handled focus being exactly at a known boundary. - Focus restore checks opener.isConnected before calling focus(): the delete/ clear flows this targets often remove the element that opened the dialog, and a disconnected node's focus() silently no-ops. - Hoisted the focusable-element selector to a module constant, and excluded input[type="hidden"] (it matched input:not([disabled]) and, at a boundary, broke the wrap). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 23:33:38 -04:00
steveandClaude Opus 4.8	bfc5d9a871	Make the canvas keyboard-reachable + trap focus in dialogs (#84 ) Build image / build-and-push (push) Successful in 9s Details Gadfly review (reusable) / review (pull_request) Successful in 7m40s Details Adversarial Review (Gadfly) / review (pull_request) Successful in 7m41s Details The arrow-key nudge handler existed but only ever acted on a POINTER selection, and nothing could select without a mouse — so the feature was unusable by exactly the keyboard users it's for. This is the scoped first slice: give the canvas a keyboard path in, and fix the Modal focus trap that every destructive confirmation goes through. Canvas: - The <svg> gets role="application" + an aria-label describing the controls, and a <title> naming the garden — a screen reader now announces an interactive canvas rather than an empty graphic. - Each object <g> is a focusable role="button" with an aria-label (name + kind) and aria-pressed reflecting selection. Enter/Space selects it — the step that was missing — which makes the existing arrow-key nudge reachable. - A :focus-visible CSS rule draws a dashed accent ring on keyboard focus (and NOT on a mouse click, which is the point of :focus-visible). CSS rather than React state because onFocus on an SVG <g> is unreliable, and a CSS rule cleanly overrides the shape's inline stroke. Modal (blast radius: DeleteGarden/ClearBed/DeletePlant/DeleteSeedLot/Share): - Tab is trapped inside the dialog and wraps at the ends, instead of walking out into the page behind the backdrop. - On close, focus returns to the element that opened the dialog rather than landing on <body>. Verified live against the built binary with real keyboard input: Tab focuses an object (SVG <g tabindex> genuinely takes focus), Enter flips aria-pressed false→true, the focus-visible dash renders (computed stroke-dasharray "5px, 4px"), the dialog traps focus through 5 Tabs, and Escape closes it and restores focus to the opener. Follow-ups noted, not done here: object dimensions in the aria-label (needs the garden's unit context this component doesn't hold), roving-tabindex between plops inside a focused bed, and the EditorRail tablist semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 23:20:47 -04:00
steveandClaude Opus 4.8	84f249a774	CLAUDE.md: one Gadfly sweep, not a re-review loop Build image / build-and-push (push) Successful in 18s Details Remove the "comment @gadfly review before merge" guidance — it contradicted the standing rule (take the initial review, fix, merge when green) and led to a multi-round re-review loop that dragged a small PR out for an hour. Keep the re-run mechanics as a parenthetical for the rare manual case. Steve's call, 2026-07-22. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 22:56:01 -04:00
steve	a13acedd90	Admin-gated Settings: runtime model selection, is_admin enforced (#90 ) Build image / build-and-push (push) Successful in 11s Details Closes #79. Agent model + on/off move into an admin-only Settings section, and is_admin is enforced for the first time. The live Runner is hot-swapped behind an atomic.Pointer with routes always registered, so a settings change takes effect with no restart; /capabilities reads the pointer. Secrets stay in the env, never the DB. Precedence: Settings → env → default. Verified live: swap on/off/model, bad spec → 400, race-clean. Gadfly blocking round addressed (dead code removed, error-swallowing fixed, frontend 503 handling, holder dedup).	2026-07-22 02:53:55 +00:00
steve	15734c9195	REST fill/clear on objects; clear-bed becomes one change set (#89 ) Build image / build-and-push (push) Successful in 10s Details Closes #82. Bed filling now exists without an LLM configured, and clear-bed is ONE change set instead of one per plop (a 40-plop bed no longer needs 40 undos). POST /objects/:id/fill (region by compass name or rect, degenerate rect → 400) and /clear, thin adapters over the service methods the agent already uses. Also fixed DESIGN.md's API block, which had dropped the unauthenticated public route.	2026-07-22 02:53:27 +00:00
steve	48057fe1f3	API-level tests for the /seed-lots route group (#88 ) Build image / build-and-push (push) Successful in 7s Details Closes #83 (seed-lots half). The only handler file without a sibling API test — the exact state the journal routes shipped unreachable in. Full CRUD through the router, derived-remaining (incl. negative/over-planted), private-to-owner ACL (404 not 403), and requireAuth. Verified the tests catch an unregistered route.	2026-07-22 02:53:13 +00:00
steveandClaude Opus 4.8	33e048cea9	Address second round of Gadfly findings on #89 Build image / build-and-push (push) Successful in 16s Details - Rename fillRequest → objectFillRequest to match the package's <resource><Action>Request convention (objectCreateRequest, gardenCreateRequest…). - Add the missing anonymous-clear permission case (401), mirroring anonymous fill. - Reword the makeFillPlant comment to stop referencing external branch state, which won't make sense once merged — just note the consolidation follow-up. - clearObject: send no body (undefined) rather than an empty {}, and validate the {cleared} response with a zod schema instead of a raw type cast. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 22:12:00 -04:00
steveandClaude Opus 4.8	4c4abe23c6	Use objectPlantingsPath helper instead of inline URL (#88 ) Build image / build-and-push (push) Successful in 6s Details Gadfly: I was wrong that no plantings-path helper existed — objectPlantingsPath is defined in plantings_test.go in the same package. Use it for the two planting POSTs in the remaining test. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 22:06:57 -04:00
steve	d26db3f6e3	Clear the SSE write deadline so an agent turn can outlive WriteTimeout (#87 ) Build image / build-and-push (push) Successful in 11s Details Closes #78. The 30s server WriteTimeout is an absolute deadline that was cutting every agent turn over 30s mid-stream — and the keep-alive from #73 could never work because it ticked into a connection destroyed at 30s. Refresh a per-write deadline instead: unbounded stream, bounded writes, so a stuck reader still can't pin the run goroutine. Two client-side regression tests, one per failure mode. Gadfly: no material issues on final review.	2026-07-22 02:04:22 +00:00
steveandClaude Opus 4.8	95b9d611c6	Test the deadline is refreshed per-frame, not just extended (#87 ) Build image / build-and-push (push) Successful in 10s Details Gadfly re-review: the regression test proved the stream outlives the server WriteTimeout, but its whole run was under a second — far below the 30s sseWriteTimeout — so it could NOT tell a per-frame refresh from a deadline set once at open. A revert to set-once would reintroduce the unbounded-block risk the per-frame refresh exists to prevent, and sail through the test. Make sseWriteTimeout a var (production never reassigns it) so a test can shrink it, and split into two focused cases sharing a streamFrames helper: - TestEventStreamOutlivesServerWriteTimeout — server WriteTimeout 300ms, frames straddling it, sseWriteTimeout left at 30s. Catches #78 (no deadline management → server cuts the stream). Huge margin, so CI slowness only makes it pass more surely — addresses the "timing margin tight" finding too. - TestEventStreamRefreshesDeadlinePerFrame — sseWriteTimeout shrunk to 400ms, 8 frames at a 100ms tick (800ms total, 4× margin per gap). A per-frame refresh delivers all 8; a set-once deadline expires mid-stream and cuts it. Verified each fails against its own regression: removing the per-write refresh gives 3/8 on the per-frame test (EOF at ~400ms); removing both deadline calls gives 0/3 on the outlives test. Both pass on the real code, 3× under -count. The "clear the deadline entirely" finding was already addressed by the previous commit (per-frame refresh); this covers it against reintroduction. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 20:49:57 -04:00
steveandClaude Opus 4.8	5bdaf21828	Address Gadfly findings on #88 Build image / build-and-push (push) Successful in 13s Details - Cover the plantId filter's out-of-range branch (0, -1), not just the non-numeric one — the handler rejects id < 1. - Add the documented negative-remaining case: over-planting a lot (57 against 50 bought) drives remaining to -7 through the HTTP surface. The number is a derived truth about what's committed, not a floor at zero, and that's the signal a gardener wants. - Rename objID → oid in the remaining test to match the package convention (shares_test etc.). Not taken: relocating createPlantAPI to plants_test.go — it's shared with #89's branch and the move is cleanest once both land (consolidating with that branch's makeFillPlant), noted on both PRs. The 409 "current" assertion style is deliberate and reads clearly; matching journal_test.go's exact phrasing isn't worth a divergence churn. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 20:39:43 -04:00
steveandClaude Opus 4.8	7a6f6963f9	Address Gadfly findings on #89 Build image / build-and-push (push) Successful in 23s Details - Reject a degenerate (zero-area or inverted) fill rect with 400 instead of silently planting a single plop at the object centre. An empty `"rect": {}` decodes to all-zeros and is caught the same way. New fillRect.degenerate() and a table test covering {}, zero-width, zero-height, and inverted. - Make the rect a named type (fillRect) rather than an anonymous inline struct, matching the rest of internal/api, and bind the pointer to a local in the handler so the deref is visibly guarded rather than reading req.Rect.MinX under an invariant from a line above. - ops_test: drop gid from the unpack instead of the `_ = gid` shim. - ClearBedModal: drop the `const n = plopCount` no-op alias. Not taken: moving createPlantAPI/makeFillPlant to a shared helper — that consolidation spans this branch and #88 and is cleanest once both land, as noted in the PR. Reusing an objectPlantingsPath helper: there isn't one in this package, and the one inline use doesn't earn a new helper on its own. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 20:36:45 -04:00
steveandClaude Opus 4.8	04cdf815df	Bound each SSE write instead of removing the deadline entirely Build image / build-and-push (push) Successful in 8s Details Gadfly, security lens: clearing the write deadline outright traded the truncation bug for an unbounded one. With no deadline, a client that stops reading fills the socket buffer and blocks Write forever — pinning the agent run goroutine and this stream's mutex, which also takes down the keep-alive since it needs the same lock. The old 30s WriteTimeout at least bounded that. Refresh the deadline per frame instead: the stream as a whole is unbounded, but no single write is. That is the only shape that satisfies both ends, since an absolute deadline cuts long turns and no deadline cannot be recovered from. The probe stays in openEventStream so a writer that cannot take deadlines is reported once rather than once per frame; the per-write call deliberately ignores its error for the same reason. Also widened the test's timing margins, per the second finding. tick is now GREATER than writeTimeout, so every frame lands after the deadline has already expired and the margin only ever grows — a loaded CI runner pushes this toward passing rather than toward flaking. Sized the other way it would flake exactly when CI is busiest. Passes 3/3 with -count=3. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 18:23:31 -04:00
steveandClaude Opus 4.8	3cfa72cb25	REST fill/clear on objects; clear-bed becomes one change set (#82 ) Build image / build-and-push (push) Successful in 18s Details Gadfly review (reusable) / review (pull_request) Successful in 5m18s Details Adversarial Review (Gadfly) / review (pull_request) Successful in 5m19s Details FillRegion and ClearObject were reachable only through the agent toolbox, so on an instance with no model configured the most valuable bulk operation in a garden planner — and the one carrying the most carefully reasoned geometry in the codebase — did not exist at all. internal/service/ops.go said these lived on *Service so "any future REST surface" would inherit the ACL checks; this is that surface. POST /objects/:id/fill takes a region EITHER by compass name ("all", "ne", "south half") or as an explicit rect in the object's local frame, and refuses both-or-neither: silently preferring one would make a client bug look like a geometry bug. It answers 200 rather than 201 because a fill can legitimately create nothing (the region is already planted) and there is no single resource to point a Location at. POST /objects/:id/clear replaces a client-side loop of PATCHes. That loop wrote one change set per plop, so clearing a 40-plop bed took 40 presses of Undo to put back — while the agent's clear_object, for the identical user-facing action, undid in one click. CLAUDE.md states the rule it violated: multi-row operations record together so they undo as one unit. Moving it server-side also removes the partial-failure case the loop had to reconcile. TestClearObjectIsOneChangeSetAPI pins that: fill a bed, count change sets, clear it, assert the count rose by exactly one. DESIGN.md's API block gains the two new routes, and the four it was already missing: GET/POST/DELETE /gardens/:id/share-link and the unauthenticated GET /public/gardens/:token. An unauthenticated surface absent from the architecture doc is the one worth fixing on sight. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 18:20:54 -04:00
steveandClaude Opus 4.8	8552f1d152	API-level tests for the /seed-lots route group (#83 ) Build image / build-and-push (push) Successful in 7s Details Gadfly review (reusable) / review (pull_request) Successful in 6m32s Details Adversarial Review (Gadfly) / review (pull_request) Successful in 6m32s Details Every other handler file had a sibling API test; this group had none, which is the state PATCH/DELETE /journal/:id shipped in — implemented, unit-tested through the service, and completely unreachable. Service tests cannot see a route that was never registered or one registered with the wrong :param. Four tests, covering what a broken route would silently take with it: - Full CRUD over HTTP, including GET/PATCH/DELETE by id, the ?plantId= filter, a rejected non-numeric plantId, and a stale-version 409 carrying the current row so the client can rebase. - `remaining` is derived through the HTTP surface, not just in the service. DESIGN.md makes derivation load-bearing, and the number is the whole reason anyone opens the seed shelf. - Lots are private to the buyer: another user gets 404 (not 403 — existence is masked per the project convention) on get/patch/delete, sees none in their listing, and the owner still has access afterwards. - The group is behind requireAuth: unauthenticated calls 401 rather than returning an empty list. Verified the tests actually catch the failure they exist for by unregistering GET /seed-lots/:id — all four fail with "404 page not found", the journal signature. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 18:16:40 -04:00
steveandClaude Opus 4.8	7088f382bc	Clear the SSE write deadline so a turn can outlive WriteTimeout (#78 ) Build image / build-and-push (push) Successful in 5s Details Gadfly review (reusable) / review (pull_request) Canceled after 9m11s Details Adversarial Review (Gadfly) / review (pull_request) Canceled after 9m11s Details http.Server.WriteTimeout is an ABSOLUTE deadline from when the request header was read, not an idle timeout. pansy sets it to 30s (cmd/pansy/main.go:65) while an agent turn is budgeted 4 minutes, so every turn over 30 seconds was cut mid-stream. The 4-minute runTimeout was unreachable; the real ceiling was 30 seconds. That also meant the keep-alive added in #73 could never do its job. It ticks at 20s, so it got exactly one tick before the connection it was pacing was destroyed underneath it — a mechanism built to survive long silences that was structurally incapable of surviving them. The failure is invisible from the handler: writes made after the deadline return err == nil and their bytes are discarded. There is no error to check and none to log, which is why this survived a review that specifically looked at the discarded write error. Only the client sees it, as an unexpected EOF, which web/src/lib/agent.ts reports as "The connection dropped partway through." — indistinguishable from a real network fault. Because it is invisible server-side, the test drives a real http.Server with a short WriteTimeout and asserts from the CLIENT side. Against the unfixed code it gets 1 of 4 frames and an unexpected EOF; a test that inspected the return of io.WriteString would have passed against the bug. gin 1.10.1's responseWriter implements Unwrap, so ResponseController reaches the underlying conn. A failure to clear the deadline IS worth logging: it means the stream will be cut and we cannot prevent it. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 18:14:12 -04:00