REST fill/clear on objects; clear-bed becomes one change set #89

Merged
steve merged 3 commits from feat/fill-clear-rest into main 2026-07-22 02:53:28 +00:00
3 changed files with 16 additions and 10 deletions
Showing only changes of commit 33e048cea9 - Show all commits
+3 -3
View File
@@ -37,14 +37,14 @@ func (r fillRect) degenerate() bool {
return r.MaxX <= r.MinX || r.MaxY <= r.MinY
}
// fillRequest is the body for POST /objects/:id/fill.
// 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 fillRequest struct {
type objectFillRequest struct {
Outdated
Review

🟡 fillRequest naming doesn't follow resourceActionRequest convention

maintainability · flagged by 1 model

  • internal/api/ops.go:47fillRequest (and its nested fillRect) should follow the established resourceActionRequest naming convention used by every other request struct in internal/api/ (e.g. gardenCreateRequest, plantCreateRequest, objectCreateRequest, journalCreateRequest). The comment says the type is named "to match the rest of internal/api", but the name itself doesn't match that pattern. Future readers have to hunt for fillRequest instead of guessing `objectFillRequest…

🪰 Gadfly · advisory

🟡 **fillRequest naming doesn't follow resourceActionRequest convention** _maintainability · flagged by 1 model_ * `internal/api/ops.go:47` — `fillRequest` (and its nested `fillRect`) should follow the established `resourceActionRequest` naming convention used by every other request struct in `internal/api/` (e.g. `gardenCreateRequest`, `plantCreateRequest`, `objectCreateRequest`, `journalCreateRequest`). The comment says the type is named "to match the rest of internal/api", but the name itself doesn't match that pattern. Future readers have to hunt for `fillRequest` instead of guessing `objectFillRequest… <sub>🪰 Gadfly · advisory</sub>
PlantID int64 `json:"plantId" binding:"required"`
Region string `json:"region"`
Rect *fillRect `json:"rect"`
@@ -58,7 +58,7 @@ func (h *handlers) fillObject(c *gin.Context) {
if !ok {
return
}
var req fillRequest
var req objectFillRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a plantId and a region are required")
return
+7 -5
View File
@@ -10,11 +10,10 @@ import (
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.
// makeFillPlant creates a custom plant and returns its id. (A near-identical
Outdated
Review

🟡 makeFillPlant comment references non-existent file and external branch state

maintainability · flagged by 1 model

  • internal/api/ops_test.go:13-17 — The comment justifying makeFillPlant's name references a seed_lots_test.go that does not exist in this branch and an external PR (#88). Once this PR lands, the comment becomes misleading stale context for anyone reading the file. Also, the package already has createGardenAPI (verb create); makeFillPlant introduces a new make prefix for the same pattern. Suggested fix: either name it createPlantAPI to match createGardenAPI, or replace the branc…

🪰 Gadfly · advisory

🟡 **makeFillPlant comment references non-existent file and external branch state** _maintainability · flagged by 1 model_ * `internal/api/ops_test.go:13-17` — The comment justifying `makeFillPlant`'s name references a `seed_lots_test.go` that does not exist in this branch and an external PR (#88). Once this PR lands, the comment becomes misleading stale context for anyone reading the file. Also, the package already has `createGardenAPI` (verb `create`); `makeFillPlant` introduces a new `make` prefix for the same pattern. Suggested fix: either name it `createPlantAPI` to match `createGardenAPI`, or replace the branc… <sub>🪰 Gadfly · advisory</sub>
// 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{
@@ -221,4 +220,7 @@ func TestFillClearPermissionsAPI(t *testing.T) {
if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, nil); w.Code != http.StatusUnauthorized {
t.Errorf("anonymous fill = %d, want 401", w.Code)
Review

🟡 Missing anonymous clear permission test — only fill is tested, not clear

error-handling · flagged by 1 model

  • internal/api/ops_test.go:221TestFillClearPermissionsAPI tests anonymous fill (expects 401) but never tests anonymous clear. The description says "anonymous gets 401" for both operations; the test should cover both endpoints to prevent a future regression where one route loses its auth middleware. The route is behind requireAuth() in api.go, so the behaviour is correct — this is a test-coverage gap, not a runtime bug.

🪰 Gadfly · advisory

🟡 **Missing anonymous clear permission test — only fill is tested, not clear** _error-handling · flagged by 1 model_ - **`internal/api/ops_test.go:221`** — `TestFillClearPermissionsAPI` tests anonymous `fill` (expects 401) but never tests anonymous `clear`. The description says "anonymous gets 401" for both operations; the test should cover both endpoints to prevent a future regression where one route loses its auth middleware. The route is behind `requireAuth()` in `api.go`, so the behaviour is correct — this is a test-coverage gap, not a runtime bug. <sub>🪰 Gadfly · advisory</sub>
}
if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, nil); w.Code != http.StatusUnauthorized {
t.Errorf("anonymous clear = %d, want 401", w.Code)
}
}
+6 -2
View File
@@ -328,6 +328,8 @@ export function useUpdatePlanting(gardenId: number) {
})
}
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
@@ -341,8 +343,10 @@ export function useClearObject(gardenId: number) {
const qc = useQueryClient()
return useMutation({
Outdated
Review

🟡 clearObject POST sends empty body {} instead of undefined and uses raw type cast

maintainability · flagged by 1 model

  • web/src/lib/objects.ts:344api.post(\/objects/${objectId}/clear`, {}) sends an empty JSON object body when the endpoint doesn't read a body at all. The codebase's other no-body POST (useRevertChangeSetinhistory.ts) passes undefined, which omits the body and the content-typeheader. The{}is inconsistent and slightly misleading about the API contract. Also, the response is cast withas { cleared?: number }` rather than parsed through a zod schema, which the codebase increa…

🪰 Gadfly · advisory

🟡 **clearObject POST sends empty body {} instead of undefined and uses raw type cast** _maintainability · flagged by 1 model_ * `web/src/lib/objects.ts:344` — `api.post(\`/objects/${objectId}/clear\`, {})` sends an empty JSON object body when the endpoint doesn't read a body at all. The codebase's other no-body POST (`useRevertChangeSet` in `history.ts`) passes `undefined`, which omits the body and the `content-type` header. The `{}` is inconsistent and slightly misleading about the API contract. Also, the response is cast with `as { cleared?: number }` rather than parsed through a zod schema, which the codebase increa… <sub>🪰 Gadfly · advisory</sub>
mutationFn: async (objectId: number): Promise<number> => {
const res = (await api.post(`/objects/${objectId}/clear`, {})) as { cleared?: number }
return res?.cleared ?? 0
// 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
},
onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),
onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')),