From 3cfa72cb25eebc0583d5bddf78df90104d3891e1 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 18:20:54 -0400 Subject: [PATCH 1/3] REST fill/clear on objects; clear-bed becomes one change set (#82) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- DESIGN.md | 4 + internal/api/api.go | 5 + internal/api/ops.go | 96 +++++++++++++ internal/api/ops_test.go | 211 +++++++++++++++++++++++++++++ web/src/editor/ClearBedModal.tsx | 10 +- web/src/lib/objects.ts | 30 ++-- web/src/pages/GardenEditorPage.tsx | 3 +- 7 files changed, 337 insertions(+), 22 deletions(-) create mode 100644 internal/api/ops.go create mode 100644 internal/api/ops_test.go diff --git a/DESIGN.md b/DESIGN.md index 052426e..ad41caa 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -64,6 +64,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) @@ -72,6 +74,8 @@ POST /agent/chat ← SSE: step events, then the finished turn (edit GET,DELETE /gardens/:id/agent/history (the actor's own thread) GET /capabilities ← what this instance can do, so the UI offers only what works 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. diff --git a/internal/api/api.go b/internal/api/api.go index 8777f11..5d88367 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -119,6 +119,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. diff --git a/internal/api/ops.go b/internal/api/ops.go new file mode 100644 index 0000000..a59c464 --- /dev/null +++ b/internal/api/ops.go @@ -0,0 +1,96 @@ +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. + +// fillRequest 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 fillRequest struct { + PlantID int64 `json:"plantId" binding:"required"` + Region string `json:"region"` + Rect *struct { + MinX float64 `json:"minXCm"` + MinY float64 `json:"minYCm"` + MaxX float64 `json:"maxXCm"` + MaxY float64 `json:"maxYCm"` + } `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 fillRequest + 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 named { + created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM) + } else { + region := service.Region{MinX: req.Rect.MinX, MinY: req.Rect.MinY, MaxX: req.Rect.MaxX, MaxY: req.Rect.MaxY} + created, err = h.svc.FillRegion(c.Request.Context(), actor, id, 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}) +} diff --git a/internal/api/ops_test.go b/internal/api/ops_test.go new file mode 100644 index 0000000..a505016 --- /dev/null +++ b/internal/api/ops_test.go @@ -0,0 +1,211 @@ +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. +// +// Deliberately named apart from the seed-lot tests' helper: that one arrives on +// a separate branch, and two files declaring the same helper would collide on +// whichever merged second. +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, "fill@example.com") + gid, 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) + } + _ = gid +} + +// 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, "region@example.com") + _, 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 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, "onecs@example.com") + 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, "owner-fill@example.com") + viewer := registerAndCookie(t, r, "viewer-fill@example.com") + stranger := registerAndCookie(t, r, "stranger-fill@example.com") + + gid, objID, plantID := seedFillableBed(t, r, owner, 200, 200, 20) + if w := doJSON(t, r, http.MethodPost, sharesPath(gid), + map[string]any{"email": "viewer-fill@example.com", "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) + } +} diff --git a/web/src/editor/ClearBedModal.tsx b/web/src/editor/ClearBedModal.tsx index ed5665e..81b8e9d 100644 --- a/web/src/editor/ClearBedModal.tsx +++ b/web/src/editor/ClearBedModal.tsx @@ -5,18 +5,20 @@ 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 + const n = plopCount return (
@@ -32,7 +34,7 @@ export function ClearBedModal({ type="button" variant="danger" disabled={clear.isPending || n === 0} - onClick={() => clear.mutate(plops, { onSuccess: onClose })} + onClick={() => clear.mutate(objectId, { onSuccess: onClose })} > {clear.isPending ? 'Clearing…' : 'Clear bed'} diff --git a/web/src/lib/objects.ts b/web/src/lib/objects.ts index ba391ec..21cf433 100644 --- a/web/src/lib/objects.ts +++ b/web/src/lib/objects.ts @@ -328,28 +328,24 @@ 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). */ +/** 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 => { + const res = (await api.post(`/objects/${objectId}/clear`, {})) as { cleared?: number } + return res?.cleared ?? 0 }, - // 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.')), }) } diff --git a/web/src/pages/GardenEditorPage.tsx b/web/src/pages/GardenEditorPage.tsx index 878766a..b723cb0 100644 --- a/web/src/pages/GardenEditorPage.tsx +++ b/web/src/pages/GardenEditorPage.tsx @@ -526,8 +526,9 @@ export function GardenEditorPage() { {clearing && focusedObject && ( ({ id: p.id, version: p.version }))} + plopCount={focusedPlops.length} gardenId={gid} onClose={() => setClearing(false)} /> From 7a6f6963f95c2c52bf91d3e7b2ccb89badf38dcc Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 20:36:45 -0400 Subject: [PATCH 2/3] Address Gadfly findings on #89 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/api/ops.go | 44 +++++++++++++++++++++++--------- internal/api/ops_test.go | 17 ++++++++++-- web/src/editor/ClearBedModal.tsx | 6 ++--- 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/internal/api/ops.go b/internal/api/ops.go index a59c464..97e0cba 100644 --- a/internal/api/ops.go +++ b/internal/api/ops.go @@ -19,6 +19,24 @@ import ( // 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 +} + // fillRequest is the body for POST /objects/:id/fill. // // A region is given EITHER by compass name ("ne", "south half", "all") or as an @@ -27,14 +45,9 @@ import ( // Exactly one must be supplied — accepting both and silently preferring one // would make a client bug look like a geometry bug. type fillRequest struct { - PlantID int64 `json:"plantId" binding:"required"` - Region string `json:"region"` - Rect *struct { - MinX float64 `json:"minXCm"` - MinY float64 `json:"minYCm"` - MaxX float64 `json:"maxXCm"` - MaxY float64 `json:"maxYCm"` - } `json:"rect"` + 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"` @@ -62,11 +75,18 @@ func (h *handlers) fillObject(c *gin.Context) { created []domain.Planting err error ) - if named { - created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM) - } else { - region := service.Region{MinX: req.Rect.MinX, MinY: req.Rect.MinY, MaxX: req.Rect.MaxX, MaxY: req.Rect.MaxY} + 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) diff --git a/internal/api/ops_test.go b/internal/api/ops_test.go index a505016..2bcd06f 100644 --- a/internal/api/ops_test.go +++ b/internal/api/ops_test.go @@ -61,7 +61,7 @@ func countChangeSets(t *testing.T, r *gin.Engine, cookie *http.Cookie, gardenID func TestFillAndClearAPI(t *testing.T) { r := authEngine(t, localCfg()) cookie := registerAndCookie(t, r, "fill@example.com") - gid, objID, plantID := seedFillableBed(t, r, cookie, 200, 200, 20) + _, objID, plantID := seedFillableBed(t, r, cookie, 200, 200, 20) // Fill by compass name. w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{ @@ -96,7 +96,6 @@ func TestFillAndClearAPI(t *testing.T) { if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != 0 { t.Errorf("second clear removed %d, want 0", n) } - _ = gid } // TestFillRegionSelectionAPI: exactly one of region/rect, and a rect fills only @@ -125,6 +124,20 @@ func TestFillRegionSelectionAPI(t *testing.T) { 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{ diff --git a/web/src/editor/ClearBedModal.tsx b/web/src/editor/ClearBedModal.tsx index 81b8e9d..bfc4b7f 100644 --- a/web/src/editor/ClearBedModal.tsx +++ b/web/src/editor/ClearBedModal.tsx @@ -18,12 +18,12 @@ export function ClearBedModal({ onClose: () => void }) { const clear = useClearObject(gardenId) - const n = plopCount return (

- Remove all {n} {n === 1 ? 'plant' : 'plants'} from{' '} + Remove all {plopCount}{' '} + {plopCount === 1 ? 'plant' : 'plants'} from{' '} {objectName}? They're marked removed but kept in history.

@@ -33,7 +33,7 @@ export function ClearBedModal({