Public read-only share link (no login / no OIDC) (#41) #43

Merged
steve merged 2 commits from feat/public-share-link into main 2026-07-19 06:18:31 +00:00
Owner

Closes #41.

A per-garden public link anyone can open read-only, with no login and no OIDC — toggle on/off, and regenerate to invalidate the old URL.

Backend

  • Migration 0004: gardens.public_token (nullable, unique over non-NULL). Rotating the token invalidates the previous URL.
  • Owner-only management (requireGardenRole(roleOwner) + CSRF): GET/POST/DELETE /gardens/:id/share-link → state / enable-or-rotate ({"rotate":true}) / disable. The token is stored raw (it must be shown back to the owner to copy) and is never selected into the normal garden payload — it stays out of gardenColumns, so it can't leak through GetGarden/GardenFull.
  • Unauthenticated GET /api/v1/public/gardens/:token → the read-only /full payload. Deliberately outside requireAuth: the token is the capability, so a logged-out visitor is never bounced to /login/OIDC. The public view drops MyRole and OwnerID to minimize what an anonymous viewer learns. Unknown / rotated / disabled / blank tokens are masked as 404 (never a redirect).
  • assembleFull is extracted from GardenFull so the authenticated and public reads return an identical shape.

Frontend

  • /g/$token route with no beforeLoad auth guard (the SPA fallback already serves any non-API GET), rendering GardenCanvas read-only (canEdit=false) via usePublicGarden — pan/zoom to explore, no editing chrome. Resets transient editor store state on mount so a logged-in user arriving from their own editor gets a clean canvas.
  • The Share dialog gains a Public link section: create, copy, regenerate, turn off.

Security notes

  • The token is a 144-bit (18-byte) URL-safe random capability; it lives in the URL path (inherent to a shareable link, like an unlisted-doc URL).
  • State-changing management endpoints stay behind auth + CSRF; only the GET read is public.

Verification

  • Backend: GOWORK=off go build/vet/test ./internal/... — incl. new TestPublicShareLink (service lifecycle + ACL) and TestPublicShareLinkAPI (owner-only management + the cookieless public read returning 200, and 404 for rotated/disabled/unknown tokens).
  • Frontend: npm run build (tsc + vite) , npm run test .

🤖 Generated with Claude Code

Closes #41. A per-garden public link anyone can open **read-only**, with **no login and no OIDC** — toggle on/off, and regenerate to invalidate the old URL. ## Backend - **Migration 0004**: `gardens.public_token` (nullable, unique over non-NULL). Rotating the token invalidates the previous URL. - **Owner-only management** (`requireGardenRole(roleOwner)` + CSRF): `GET/POST/DELETE /gardens/:id/share-link` → state / enable-or-rotate (`{"rotate":true}`) / disable. The token is stored **raw** (it must be shown back to the owner to copy) and is **never** selected into the normal garden payload — it stays out of `gardenColumns`, so it can't leak through `GetGarden`/`GardenFull`. - **Unauthenticated** `GET /api/v1/public/gardens/:token` → the read-only `/full` payload. Deliberately outside `requireAuth`: the token is the capability, so a logged-out visitor is never bounced to `/login`/OIDC. The public view drops `MyRole` and `OwnerID` to minimize what an anonymous viewer learns. Unknown / rotated / disabled / blank tokens are masked as **404** (never a redirect). - `assembleFull` is extracted from `GardenFull` so the authenticated and public reads return an identical shape. ## Frontend - **`/g/$token`** route with **no** `beforeLoad` auth guard (the SPA fallback already serves any non-API GET), rendering `GardenCanvas` read-only (`canEdit=false`) via `usePublicGarden` — pan/zoom to explore, no editing chrome. Resets transient editor store state on mount so a logged-in user arriving from their own editor gets a clean canvas. - The **Share** dialog gains a **Public link** section: create, copy, regenerate, turn off. ## Security notes - The token is a 144-bit (18-byte) URL-safe random capability; it lives in the URL path (inherent to a shareable link, like an unlisted-doc URL). - State-changing management endpoints stay behind auth + CSRF; only the GET read is public. ## Verification - Backend: `GOWORK=off go build/vet/test ./internal/...` ✅ — incl. new `TestPublicShareLink` (service lifecycle + ACL) and `TestPublicShareLinkAPI` (owner-only management + the **cookieless** public read returning 200, and 404 for rotated/disabled/unknown tokens). - Frontend: `npm run build` (tsc + vite) ✅, `npm run test` ✅. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-19 06:04:30 +00:00
Public read-only share link (#41)
Build image / build-and-push (push) Successful in 8s
Gadfly review (reusable) / review (pull_request) Successful in 10m11s
Adversarial Review (Gadfly) / review (pull_request) Successful in 10m11s
f3b0ca572d
A per-garden public link anyone can open read-only, with no login and no OIDC.

Backend:
- Migration 0004 adds gardens.public_token (nullable, unique over non-NULL).
- Owner-only management: GET/POST/DELETE /gardens/:id/share-link (state /
  enable-or-rotate / disable). The token is stored raw (it must be shown back
  to the owner) and never selected into the normal garden payload.
- Unauthenticated GET /api/v1/public/gardens/:token returns the read-only /full
  payload — the token is the capability, so no requireAuth and no redirect. The
  public view drops MyRole and OwnerID to minimize what an anonymous viewer
  learns. Unknown/rotated/disabled tokens are masked as 404.
- assembleFull is extracted from GardenFull so both reads return one shape.

Frontend:
- /g/$token route with NO auth guard (SPA fallback already serves it), rendering
  GardenCanvas read-only via usePublicGarden.
- Share dialog gains a Public link section: create, copy, regenerate, turn off.

Tests: service lifecycle + ACL (owner-only, non-owner masked, rotate/disable
invalidation) and the HTTP surface incl. the cookieless public read.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-19 06:14:41Z

claude-code/sonnet · claude-code — done

  • security — Minor issues
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

glm-5.2:cloud · ollama-cloud — done

  • security — Minor issues
  • correctness — Minor issues
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — No material issues found

kimi-k2.6:cloud · ollama-cloud — done

  • security — Minor issues
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — No material issues found

opencode/glm-5.2:cloud · opencode — done

  • security — Minor issues
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • ⚠️ error-handling — could not complete

opencode/kimi-k2.6:cloud · opencode — done

  • security — Blocking issues found
  • correctness — No material issues found
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — Minor issues

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 5/5 reviewers finished · updated 2026-07-19 06:14:41Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ⚠️ **error-handling** — could not complete #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — Blocking issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-19 06:14:42 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 11 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 11 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +10,4 @@
// public share token. It sits OUTSIDE requireAuth — the token itself is the
// capability — so a logged-out visitor can open a shared link without ever being
// bounced to /login or the OIDC flow. An unknown or disabled token is a 404.
func (h *handlers) getPublicGarden(c *gin.Context) {

🟠 Public garden endpoint lacks cache-control, allowing shared proxy caching of capability-URL responses

security · flagged by 1 model

  • internal/api/public.go:13getPublicGarden returns sensitive garden data for a capability-URL but sets no Cache-Control header. Without Cache-Control: private, no-store (or at minimum private), shared HTTP caches (corporate proxies, CDNs, ISP gateways) may cache the response keyed by the secret token URL. This extends data exposure beyond token rotation/disablement and stores the capability in intermediate infrastructure. Fix: Add `c.Header("Cache-Control", "private, no-stor…

🪰 Gadfly · advisory

🟠 **Public garden endpoint lacks cache-control, allowing shared proxy caching of capability-URL responses** _security · flagged by 1 model_ - **`internal/api/public.go:13`** — `getPublicGarden` returns sensitive garden data for a capability-URL but sets no `Cache-Control` header. Without `Cache-Control: private, no-store` (or at minimum `private`), shared HTTP caches (corporate proxies, CDNs, ISP gateways) may cache the response keyed by the secret token URL. This extends data exposure beyond token rotation/disablement and stores the capability in intermediate infrastructure. **Fix:** Add `c.Header("Cache-Control", "private, no-stor… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +16,4 @@
writeServiceError(c, err)
return
}
c.JSON(http.StatusOK, full)

🟡 Public garden read has no HTTP cache headers despite being the many-viewer hot path (4 DB queries per load)

performance · flagged by 1 model

Confirmed. internal/api/public.go:19 returns via c.JSON with no cache headers; assembleFull (objects.go:174-186) runs 3 indexed queries plus the GetGardenByPublicToken lookup = 4 DB queries per load; only spa.go sets Cache-Control in the API package; web/src/lib/publicGarden.ts:16 has staleTime: 30_000. The finding survives.

🪰 Gadfly · advisory

🟡 **Public garden read has no HTTP cache headers despite being the many-viewer hot path (4 DB queries per load)** _performance · flagged by 1 model_ Confirmed. `internal/api/public.go:19` returns via `c.JSON` with no cache headers; `assembleFull` (objects.go:174-186) runs 3 indexed queries plus the `GetGardenByPublicToken` lookup = 4 DB queries per load; only `spa.go` sets `Cache-Control` in the API package; `web/src/lib/publicGarden.ts:16` has `staleTime: 30_000`. The finding survives. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +45,4 @@
var req struct {
Rotate bool `json:"rotate"`
}
_ = c.ShouldBindJSON(&req)

🟡 createShareLink silently discards malformed JSON body instead of returning 400

error-handling · flagged by 2 models

  • internal/api/public.go:48 (createShareLink) — _ = c.ShouldBindJSON(&req) discards the bind error entirely, not just for a missing body. If a client sends a malformed JSON body (e.g. a syntax error from a client bug) rather than no body at all, ShouldBindJSON fails, req is left at its zero value, and the handler silently proceeds as rotate=false and returns 200 OK. The doc comment frames this as "missing/malformed just means enable-without-rotate," so it's an intentional desig…

🪰 Gadfly · advisory

🟡 **createShareLink silently discards malformed JSON body instead of returning 400** _error-handling · flagged by 2 models_ - `internal/api/public.go:48` (`createShareLink`) — `_ = c.ShouldBindJSON(&req)` discards the bind error entirely, not just for a missing body. If a client sends a **malformed** JSON body (e.g. a syntax error from a client bug) rather than no body at all, `ShouldBindJSON` fails, `req` is left at its zero value, and the handler silently proceeds as `rotate=false` and returns `200 OK`. The doc comment frames this as "missing/malformed just means enable-without-rotate," so it's an intentional desig… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +64,4 @@
writeServiceError(c, err)
return
}
c.JSON(http.StatusOK, gin.H{"enabled": false})

🟡 deleteShareLink hand-builds gin.H instead of returning typed PublicShareLink; response shape can drift

maintainability · flagged by 1 model

  • internal/api/public.go:67 returns gin.H{"enabled": false} while getShareLink/createShareLink return the typed *PublicShareLink. The disable response shape is now hand-built in the handler and will silently fall out of sync with PublicShareLink if it gains a field. The service's DisablePublicShareLink returns only error, so either have it return the resulting disabled *PublicShareLink, or have the handler return &PublicShareLink{Enabled: false}. Verified by reading the f…

🪰 Gadfly · advisory

🟡 **deleteShareLink hand-builds gin.H instead of returning typed PublicShareLink; response shape can drift** _maintainability · flagged by 1 model_ - **`internal/api/public.go:67` returns `gin.H{"enabled": false}`** while `getShareLink`/`createShareLink` return the typed `*PublicShareLink`. The disable response shape is now hand-built in the handler and will silently fall out of sync with `PublicShareLink` if it gains a field. The service's `DisablePublicShareLink` returns only `error`, so either have it return the resulting disabled `*PublicShareLink`, or have the handler return `&PublicShareLink{Enabled: false}`. Verified by reading the f… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +33,4 @@
g, err := s.store.GetGardenByPublicToken(ctx, token)
if err != nil {
return nil, err
}

🟠 Public read scrubs garden.OwnerID but plants still leak the owner's user id via Plant.OwnerID

correctness, security · flagged by 2 models

  • internal/service/public.go:36-44 — The public read scrubs full.Garden.OwnerID and MyRole, but full.Plants still carries each plant's OwnerID (domain.Plant.OwnerID *int64, json ownerId,omitempty, per internal/domain/domain.go:186). ListReferencedPlants selects owner_id (internal/store/plants.go:13 / plantColumns) and scans it into p.OwnerID (internal/store/plants.go:19), so the field is populated. For a garden whose owner created the referenced plants (the common cas…

🪰 Gadfly · advisory

🟠 **Public read scrubs garden.OwnerID but plants still leak the owner's user id via Plant.OwnerID** _correctness, security · flagged by 2 models_ - `internal/service/public.go:36-44` — The public read scrubs `full.Garden.OwnerID` and `MyRole`, but `full.Plants` still carries each plant's `OwnerID` (`domain.Plant.OwnerID *int64`, json `ownerId,omitempty`, per `internal/domain/domain.go:186`). `ListReferencedPlants` selects `owner_id` (`internal/store/plants.go:13` / `plantColumns`) and scans it into `p.OwnerID` (`internal/store/plants.go:19`), so the field is populated. For a garden whose owner created the referenced plants (the common cas… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +40,4 @@
}
// Minimize what an anonymous viewer learns: no role, and don't expose the
// owner's user id (the page renders fine without it).
full.Garden.MyRole = ""

🟠 Plant.OwnerID leaks in public payload despite Garden.OwnerID sanitization

security · flagged by 3 models

  • internal/service/public.go:43-44PublicGarden clears OwnerID on the top-level garden but not on nested Plants. assembleFull loads referenced plants via ListReferencedPlants, and each domain.Plant carries an OwnerID *int64 (the user ID of the plant's creator). For custom plants this leaks the owner's user ID — exactly the kind of identity disclosure the PR says it prevents by zeroing Garden.OwnerID. Worse, if a co-editor created a planting with their own custom plant…

🪰 Gadfly · advisory

🟠 **Plant.OwnerID leaks in public payload despite Garden.OwnerID sanitization** _security · flagged by 3 models_ - **`internal/service/public.go:43-44`** — `PublicGarden` clears `OwnerID` on the top-level garden but **not on nested `Plants`**. `assembleFull` loads referenced plants via `ListReferencedPlants`, and each `domain.Plant` carries an `OwnerID *int64` (the user ID of the plant's creator). For custom plants this leaks the owner's user ID — exactly the kind of identity disclosure the PR says it prevents by zeroing `Garden.OwnerID`. Worse, if a co-editor created a planting with their own custom plant… <sub>🪰 Gadfly · advisory</sub>
@@ -133,0 +154,4 @@
const url = token ? `${window.location.origin}/g/${token}` : ''
const busy = link.isPending || enable.isPending || disable.isPending
const run = (p: Promise<unknown>, fallback: string) => {

Second ad-hoc mutation-error-to-state helper added alongside existing onMutationError in the same file

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **Second ad-hoc mutation-error-to-state helper added alongside existing onMutationError in the same file** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -133,0 +164,4 @@
try {
await navigator.clipboard.writeText(url)
setCopied(true)
window.setTimeout(() => setCopied(false), 1500)

🟡 Missing setTimeout cleanup in copy() can set state on unmounted component

error-handling · flagged by 1 model

  • web/src/components/gardens/ShareGardenModal.tsx:167 — Missing cleanup for the setTimeout in copy(). If PublicLinkSection unmounts before the 1500 ms timeout fires (e.g., user closes the Share dialog right after clicking Copy), React will warn about a state update on an unmounted component. Multiple rapid clicks on Copy also create overlapping timeouts that can prematurely clear the copied state. Fix: Track the timeout ID in a ref and clear it both before starting a new one and…

🪰 Gadfly · advisory

🟡 **Missing setTimeout cleanup in copy() can set state on unmounted component** _error-handling · flagged by 1 model_ - **`web/src/components/gardens/ShareGardenModal.tsx:167`** — Missing cleanup for the `setTimeout` in `copy()`. If `PublicLinkSection` unmounts before the 1500 ms timeout fires (e.g., user closes the Share dialog right after clicking Copy), React will warn about a state update on an unmounted component. Multiple rapid clicks on Copy also create overlapping timeouts that can prematurely clear the copied state. **Fix:** Track the timeout ID in a ref and clear it both before starting a new one and… <sub>🪰 Gadfly · advisory</sub>
@@ -71,0 +75,4 @@
export const shareLinkSchema = z.object({
enabled: z.boolean(),
token: z.string().optional(),

🟡 shareLinkSchema.token has no format/length invariant; backend regression would pass silently

maintainability · flagged by 1 model

  • web/src/lib/shares.ts:78 shareLinkSchema's token: z.string().optional() imposes no format/length invariant, while the backend generates a 24-char URL-safe token (newPublicTokenrandToken(18) → base64-rawurl) and the rest of the data layer (e.g. shareRoleSchema as z.enum) encodes its invariants in zod. A .min(1).regex(/^[A-Za-z0-9_-]+$/) would make the contract explicit and catch a backend regression. Verified by reading the file and internal/service/service.go.

🪰 Gadfly · advisory

🟡 **shareLinkSchema.token has no format/length invariant; backend regression would pass silently** _maintainability · flagged by 1 model_ - **`web/src/lib/shares.ts:78` `shareLinkSchema`'s `token: z.string().optional()`** imposes no format/length invariant, while the backend generates a 24-char URL-safe token (`newPublicToken` → `randToken(18)` → base64-rawurl) and the rest of the data layer (e.g. `shareRoleSchema` as `z.enum`) encodes its invariants in zod. A `.min(1).regex(/^[A-Za-z0-9_-]+$/)` would make the contract explicit and catch a backend regression. Verified by reading the file and `internal/service/service.go`. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +24,4 @@
// Start from a clean canvas: if a logged-in user reaches here from their own
// editor, stale focus/selection state would otherwise dim or highlight things.
useEffect(() => {

🟠 Manual store reset omits objectDragging and is fragile to future store additions

maintainability · flagged by 4 models

  • web/src/pages/PublicGardenPage.tsx:27-36 — The mount effect manually enumerates seven individual useEditorStore setters to reset transient editor state. It omits objectDragging, so a logged-in user arriving from an active editor gesture (where objectDragging may be true) could find pan/zoom broken on the public canvas because the viewport stands down while that flag is set. More broadly, manually listing fields is fragile: any new transient field added to the editor store will be…

🪰 Gadfly · advisory

🟠 **Manual store reset omits objectDragging and is fragile to future store additions** _maintainability · flagged by 4 models_ - **`web/src/pages/PublicGardenPage.tsx:27-36`** — The mount effect manually enumerates seven individual `useEditorStore` setters to reset transient editor state. It omits `objectDragging`, so a logged-in user arriving from an active editor gesture (where `objectDragging` may be `true`) could find pan/zoom broken on the public canvas because the viewport stands down while that flag is set. More broadly, manually listing fields is fragile: any new transient field added to the editor store will be… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +35,4 @@
s.setLivePlanting(null)
}, [token])
const objects = useMemo(() => full.data?.objects.map(toEditorObject) ?? [], [full.data?.objects])

🟠 Editor view-model memos + EditorGarden mapping duplicated from GardenEditorPage; extract toEditorGarden/useEditorViewModel

maintainability · flagged by 1 model

  • web/src/pages/PublicGardenPage.tsx:38 and :52 duplicate GardenEditorPage.tsx:55-60 and :219-226 — the objects/plantings/plants/plantsById memo block (functionally identical; the editor page just routes through intermediate serverObjects/serverPlantings locals) and the five-field EditorGarden copy from full.data.garden (line-for-line identical). This is the canonical "build editor view-model from FullGarden" transform; both pages should share a `useEditorViewModel(fu…

🪰 Gadfly · advisory

🟠 **Editor view-model memos + EditorGarden mapping duplicated from GardenEditorPage; extract toEditorGarden/useEditorViewModel** _maintainability · flagged by 1 model_ - **`web/src/pages/PublicGardenPage.tsx:38` and `:52` duplicate `GardenEditorPage.tsx:55-60` and `:219-226`** — the `objects`/`plantings`/`plants`/`plantsById` memo block (functionally identical; the editor page just routes through intermediate `serverObjects`/`serverPlantings` locals) and the five-field `EditorGarden` copy from `full.data.garden` (line-for-line identical). This is the canonical "build editor view-model from FullGarden" transform; both pages should share a `useEditorViewModel(fu… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Blocking issues found · 11 findings (4 with multi-model agreement)

Finding Where Models Lens
🟠 Manual store reset omits objectDragging and is fragile to future store additions web/src/pages/PublicGardenPage.tsx:27 4/5 maintainability
🟠 Plant.OwnerID leaks in public payload despite Garden.OwnerID sanitization internal/service/public.go:43 3/5 security
🟠 Public read scrubs garden.OwnerID but plants still leak the owner's user id via Plant.OwnerID internal/service/public.go:36 2/5 correctness, security
🟡 createShareLink silently discards malformed JSON body instead of returning 400 internal/api/public.go:48 2/5 error-handling
7 single-model findings (lower confidence)
Finding Where Model Lens
🟠 Public garden endpoint lacks cache-control, allowing shared proxy caching of capability-URL responses internal/api/public.go:13 kimi-k2.6:cloud security
🟠 Editor view-model memos + EditorGarden mapping duplicated from GardenEditorPage; extract toEditorGarden/useEditorViewModel web/src/pages/PublicGardenPage.tsx:38 opencode/glm-5.2:cloud maintainability
🟡 Public garden read has no HTTP cache headers despite being the many-viewer hot path (4 DB queries per load) internal/api/public.go:19 glm-5.2:cloud performance
🟡 deleteShareLink hand-builds gin.H instead of returning typed PublicShareLink; response shape can drift internal/api/public.go:67 opencode/glm-5.2:cloud maintainability
🟡 Missing setTimeout cleanup in copy() can set state on unmounted component web/src/components/gardens/ShareGardenModal.tsx:167 opencode/kimi-k2.6:cloud error-handling
🟡 shareLinkSchema.token has no format/length invariant; backend regression would pass silently web/src/lib/shares.ts:78 opencode/glm-5.2:cloud maintainability
Second ad-hoc mutation-error-to-state helper added alongside existing onMutationError in the same file web/src/components/gardens/ShareGardenModal.tsx:157 claude-code/sonnet maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

These supporting claims all check out and are consistent with the diff. The one finding is solidly confirmed by the actual source. My review is complete.

VERDICT: Minor issues

  • internal/service/public.go:37-38 (PublicGarden) — the anonymous-viewer redaction is incomplete. It zeroes full.Garden.MyRole and full.Garden.OwnerID, but full.Plants (from ListReferencedPlants, internal/store/plants.go:31-57) is passed through unredacted. plantColumns/scanPlant (internal/store/plants.go:13-25) populate domain.Plant.OwnerID (internal/domain/domain.go:186, *int64, json:"ownerId,omitempty", nil only for built-ins) directly from plants.owner_id. Any custom (non-built-in) plant referenced by an active planting in the garden — created by the owner or any editor/collaborator — leaks that user's real numeric user ID to an anonymous visitor holding the link. This contradicts the function's own stated intent ("minimize what an anonymous viewer learns... don't expose the owner's user id") and is inconsistent within the same function. Confirmed the shipped test (internal/service/public_test.go) seeds a plant via seedOwnPlant(t, s, owner, 10) (owned by the real owner user) and asserts it appears in full.Plants (len(full.Plants) != 1 check) without ever checking/scrubbing OwnerID — the leak is present in the shipped fixture, just not asserted on. Fix: zero out (or omit) OwnerID on each entry in full.Plants inside PublicGarden, same as done for the garden itself.
🎯 Correctness — No material issues found

Both helper functions exist and are used identically to how GardenEditorPage.tsx uses them, so no signature mismatch there either.

VERDICT: No material issues found

I reviewed this PR through the correctness lens — logic bugs and semantic/domain correctness (constants, formulas, thresholds).

Things I specifically verified against the checked-out code:

  • Token entropy math: newPublicToken uses randToken(18) → 18×8 = 144 bits, matching the PR's claim, and base64-raw-url-encoding 18 bytes yields a 24-char string, matching the doc comment (internal/service/service.go).
  • public_token never leaks into normal reads: confirmed gardenColumns (internal/store/gardens.go:13) does not include public_token, so GetGarden/GardenFull/assembleFull can't select it.
  • assembleFull refactor (internal/service/objects.go:161-193): all three store calls now key off g.ID instead of the old gardenID parameter — same value, no behavior change; confirmed GardenFull still authorizes via requireGardenRole(..., roleViewer) before delegating.
  • PublicGarden masking of MyRole/OwnerID (internal/service/public.go:35-38): GetGardenByPublicToken never sets MyRole (only requireGardenRole does, via effectiveGardenRole), so the explicit full.Garden.MyRole = "" is redundant-but-harmless; OwnerID = 0 correctly masks the real owner ID. Garden.OwnerID json tag has no omitempty so it always serializes as 0, MyRole has omitempty so it's dropped entirely — both are consistent with what the frontend gardenSchema expects (ownerId: z.number() required, myRole optional).
  • Enable/rotate/disable idempotency logic (internal/service/public.go:71-93): token == nil || rotate correctly captures "not yet enabled" vs "force new token"; SetGardenPublicToken(ctx, gardenID, nil) correctly clears via a nil any → SQL NULL.
  • Role-check consistency: owner-only endpoints route through the same requireGardenRole(..., roleOwner) used elsewhere (shares.go), producing the same ErrForbidden-for-editor / ErrNotFound-for-stranger split the tests assert.
  • Route wiring: getShareLink/createShareLink/deleteShareLink/getPublicGarden, parseIDParam, mustActor, writeServiceError all exist with matching signatures; the public route sits inside the v1 group but csrfGuard exempts GET/HEAD/OPTIONS, so the unauthenticated read isn't accidentally blocked.
  • Frontend: PublicGardenPage builds EditorGarden and calls GardenCanvas the same way GardenEditorPage does; toEditorObject/toEditorPlanting signatures match.

Nothing here is logically wrong. The one soft observation — SetGardenPublicToken doesn't handle a UNIQUE-constraint collision on the vanishingly-unlikely case of a token collision — isn't a real correctness bug at 144 bits of entropy, so I'm not reporting it as a finding.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • web/src/pages/PublicGardenPage.tsx:27-36 and web/src/pages/GardenEditorPage.tsx:76-92: both effects reset the same six transient useEditorStore fields (select, selectPlanting, setArmedKind, setArmedPlant, setLiveObject, setLivePlanting, plus setFocusedObject) as independently-maintained call sites, confirmed by reading both files — web/src/editor/store.ts:54-81 has no shared resetTransient()-style action. The two lists already differ in order/grouping (and GardenEditorPage additionally runs the reset on unmount too), which is the kind of drift that creeps in when the same logic is copy-pasted at two sites. Low-churn fix: hoist into a single store action both pages call.

  • web/src/components/gardens/ShareGardenModal.tsx:58 vs :157-160: confirmed — the file defines onMutationError (used via mutate(..., { onError }) at lines 108/121) and a separately-invented run() helper (wraps mutateAsync in .catch(), used at lines 185/208/217), both doing nothing but setError(errorMessage(err, fallback)). Not a hard blocker given the different mutate styles at each call site, but it's a missed chance to reuse one pattern instead of adding a second.

Everything else in the draft (Go handlers/service/store following existing conventions, assembleFull extraction being an appropriately-scoped dedup) is general commentary rather than a specific verifiable claim, and nothing in it contradicts the code read.

Performance — No material issues found

Default staleTime is 30s app-wide, so shareLinkQueryOptions inherits sane caching. No refetch storms.

I reviewed the diff end-to-end through the performance lens and verified the query paths directly.

VERDICT: No material issues found

  • assembleFull (internal/service/objects.go) is a straight extraction of the existing 3-query load (ListObjectsForGarden, ListActivePlantingsForGarden, ListReferencedPlants) — verified each is a single indexed query (idx_garden_objects_garden, idx_plantings_object/idx_plantings_plant), no N+1 introduced, no behavior change vs. the pre-existing GardenFull.
  • GetGardenByPublicToken (internal/store/gardens.go:164) is a single-row lookup backed by the new partial unique index idx_gardens_public_token (0004_garden_public_token.sql:7). Equality against a non-NULL bound parameter is eligible for SQLite's partial-index optimization, so this is an indexed lookup, not a scan.
  • GetGardenPublicToken/SetGardenPublicToken are simple single-row reads/updates on the PK; SetGardenPublicToken deliberately skips bumping version, which avoids spurious extra work/conflicts for concurrent editors but has no query-count impact either way.
  • Frontend: usePublicGarden sets staleTime: 30_000 and retry: false (web/src/lib/publicGarden.ts:15), and useShareLink inherits the app-wide staleTime: 30_000 default (web/src/lib/queryClient.ts:8) — no polling or refetch storms. PublicGardenPage memoizes the objects/plantings/plants derivations off full.data, so no per-render recomputation of the full lists.
  • No loops, no per-item queries, no unbounded new payload shape beyond what the already-existing authenticated /full endpoint returns.

One observation that's adjacent to performance but really a security/access-control concern (removing the auth barrier means this bounded-but-potentially-large read is now reachable by anonymous, unrate-limited traffic) — noting it but not flagging it here since the app has no rate limiting anywhere today (this diff doesn't regress that posture, it's consistent with the rest of the API), and the auth-exposure angle itself belongs to the security lens, not mine.

Outside my lens: none worth flagging beyond the note above.

🧯 Error handling & edge cases — Minor issues

Good, confirms public_token is indeed excluded from gardenColumns, backing the "never leaks" claim in the draft's clean-list.

VERDICT: Minor issues

  • internal/api/public.go:48 (createShareLink) — _ = c.ShouldBindJSON(&req) discards the bind error entirely, not just for a missing body. If a client sends a malformed JSON body (e.g. a syntax error from a client bug) rather than no body at all, ShouldBindJSON fails, req is left at its zero value, and the handler silently proceeds as rotate=false and returns 200 OK. The doc comment frames this as "missing/malformed just means enable-without-rotate," so it's an intentional design choice, but it conflates "no opinion supplied" (fine to default) with "client tried to send something and it was broken" (every other body-parsing endpoint in this codebase — auth.go:39/60, gardens.go:52/83, shares.go:40/62, plants.go:84/102, plantings.go:83/101, objects.go:117/135 — checks the error and returns 400). Practical impact is low, but a caller intending to rotate with a serialization bug gets a silent no-op instead of an error signal.
glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

Confirmed. The finding is verified.

VERDICT: Minor issues

Through the security lens I verified the core design: the 18-byte (144-bit) crypto/rand token is the capability, the management endpoints sit behind requireAuth + requireGardenRole(roleOwner) + the csrfGuard (which rejects cross-origin state changes when PANSY_BASE_URL is set), public_token is deliberately excluded from gardenColumns so it can't leak via the normal garden payload, and unknown/rotated/disabled/blank tokens are masked as 404 via domain.ErrNotFound. The GardenFull/assembleFull split is shape-identical. SQL is parameterized (? placeholders) — no injection. The unauthenticated GET /api/v1/public/gardens/:token is correctly GET-only.

  • internal/service/public.go:43-44 (info disclosure, verified)PublicGarden zeroes full.Garden.MyRole and full.Garden.OwnerID "to minimize what an anonymous viewer learns," but the Plants slice returned by assembleFull (internal/service/objects.go:196) still carries each domain.Plant.OwnerID (internal/domain/domain.go:186, json:"ownerId,omitempty"). ListReferencedPlants selects plantColumns including owner_id (internal/store/plants.go:33). If the garden's owner has referenced any custom (non-built-in) plant — owner_id is their own user id — an anonymous viewer learns the owner's account id through plants[].ownerId, defeating the OwnerID masking the PR explicitly set out to do. Built-in plants (owner_id IS NULL, omitted from JSON) don't leak, so the exposure is conditional on custom-plant use, but it's a real, inconsistent identifier disclosure on the unauthenticated read path. Suggested fix: in PublicGarden, zero p.OwnerID for each plant in full.Plants (mirroring the OwnerID=0 treatment already applied to the garden), or have assembleFull take a public flag that strips owner identifiers from plants too.
🎯 Correctness — Minor issues

Confirmed: domain.Plant.OwnerID *int64 (json ownerId,omitempty) is populated by ListReferencedPlants (which selects owner_id via plantColumns and scans into &p.OwnerID), and CreatePlant sets p.OwnerID = &actorID. In the test's seedOwnPlant(t, s, owner, 10) case the plant's OwnerID equals the garden owner's id, while PublicGarden only zeroes full.Garden.OwnerID/MyRole and leaves full.Plants[*].OwnerID intact, so the owner's user id is re-exposed via the plants array. The finding survives.

VERDICT: Minor issues

  • internal/service/public.go:36-44 — The public read scrubs full.Garden.OwnerID and MyRole, but full.Plants still carries each plant's OwnerID (domain.Plant.OwnerID *int64, json ownerId,omitempty, per internal/domain/domain.go:186). ListReferencedPlants selects owner_id (internal/store/plants.go:13 / plantColumns) and scans it into p.OwnerID (internal/store/plants.go:19), so the field is populated. For a garden whose owner created the referenced plants (the common case — e.g. seedOwnPlant in internal/service/plantings_test.go:24, where CreatePlant sets p.OwnerID = &actorID, internal/service/plants.go:81-82), full.Plants[*].ownerId equals the garden owner's user id, so the stated goal ("don't expose the owner's user id") is only half-met: the owner ID is removed from the garden object but re-leaks through the plants array. This is a semantic-correctness gap (behavior doesn't match the documented intent). Suggested fix: zero/omit OwnerID on each plant in the PublicGarden path (or drop ownerId from the public plants payload), matching the MyRole/OwnerID scrubbing already done on the garden.
🧹 Code cleanliness & maintainability — Minor issues

Verified review

Verdict: Minor issues

  • web/src/pages/PublicGardenPage.tsx:27-36 — The mount effect imperatively resets seven fields on the shared singleton useEditorStore, duplicating the "clear transient editor state" logic that GardenEditorPage performs in its own mount effect (web/src/pages/GardenEditorPage.tsx:76-92, where the clear() closure calls select(null)/selectPlanting(null)/setArmedKind(null)/setArmedPlant(null)/setLiveObject(null)/setLivePlanting(null), with setFocusedObject handled separately). Both pages share the same store. Verified against web/src/editor/store.ts (lines 14-81): the store exposes only individual setters (select, selectPlanting, setFocusedObject, setArmedPlant, setArmedKind, setLiveObject, setLivePlanting) with no aggregate reset()/resetTransient() action, so this is copy-paste that must be kept in sync by hand whenever a new transient field is added. Maintainability hazard; should be a single shared store action called by both pages.

  • internal/api/public.go:48createShareLink discards the bind error with _ = c.ShouldBindJSON(&req). This is intentional per the function-level comment ("a missing/malformed one just means enable-without-rotate") and matches the idempotent-enable semantics, so it's defensible. Only a trivial readability nit: a one-line comment near the _ = would make the deliberate discard obvious to readers who skip the doc comment. No change required.

Performance — Minor issues

Confirmed. internal/api/public.go:19 returns via c.JSON with no cache headers; assembleFull (objects.go:174-186) runs 3 indexed queries plus the GetGardenByPublicToken lookup = 4 DB queries per load; only spa.go sets Cache-Control in the API package; web/src/lib/publicGarden.ts:16 has staleTime: 30_000. The finding survives.

Verdict: Minor issues

  • internal/api/public.go:19 — public garden read sets no HTTP cache headers, yet is the one endpoint explicitly designed for many anonymous viewers. Each page load by each visitor runs 4 SQL queries (GetGardenByPublicToken plus the three assembleFull queries in internal/service/objects.go:175-183: ListObjectsForGarden, ListActivePlantingsForGarden, ListReferencedPlants). The authenticated GardenFull path is single-user-per-garden so this didn't matter before, but a share link is specifically meant to be opened broadly and repeatedly. There's no Cache-Control/ETag on the response, so browsers/intermediaries re-hit the DB on every navigation, and react-query's staleTime: 30_000 (web/src/lib/publicGarden.ts:16) only helps within a single tab. The response is read-only and changes only on owner edits, so a short Cache-Control: public, max-age=… (or an ETag keyed on garden.updated_at/version) would cut repeated DB load for the shared-link hot path. Confirmed by grep that only spa.go sets cache headers in the API package.
🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

I reviewed the unhappy paths the diff introduces, verified against the checked-out code:

  • createShareLink swallowing c.ShouldBindJSON error (internal/api/public.go:44): _ = c.ShouldBindJSON(&req) is intentional and safe — a malformed/missing body leaves req.Rotate at its zero value false, which maps to the documented "enable without rotate" path. Not a bug.
  • PublicGarden masking fields (internal/service/public.go:23-34): blank token → ErrNotFound; GetGardenByPublicToken maps sql.ErrNoRowsErrNotFound; other store errors are wrapped and surface via the mapper. MyRole="" is omitted by the struct's omitempty, so the frontend's gardenRoleSchema.optional() parses; OwnerID=0 is a valid number for the required ownerId field. No parse/edge failure. Verified gardenSchema in web/src/lib/gardens.ts:14 and domain.Garden tags in internal/domain/domain.go:122.
  • SetGardenPublicToken nil path (internal/store/gardens.go:202): token==nilval stays the untyped nilNULL. RowsAffected()==0ErrNotFound. Correct disable/idempotent behavior.
  • Frontend PublicGardenPage guards full.isPending and full.isError before touching full.data; the ?.map(...) ?? [] memos are null-safe during pending. The clipboard catch {} in PublicLinkSection is a deliberate silent fallback with a comment.
  • run helper (ShareGardenModal.tsx) always attaches .catch, so no unhandled promise rejection; setError(null) before the async op is fine.

One trivial edge case worth noting (low confidence, not blocking):

  • internal/store/gardens.go:206 / internal/service/public.go:62: newPublicToken() generates 144 random bits and writes them under a UNIQUE index (0004_garden_public_token.sql). On the astronomically unlikely collision, the DB returns a unique-constraint violation that SetGardenPublicToken wraps but does not map to a sentinel, so writeServiceError (internal/api/errors.go:55) falls through to 500 INTERNAL. The probability is negligible at household scale, so this is not material, but a retry-on-conflict or a mapped "TOKEN_COLLISION" would be the tidy fix if you ever care.
kimi-k2.6:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

Verdict: Minor issues

  • internal/api/public.go:13getPublicGarden returns sensitive garden data for a capability-URL but sets no Cache-Control header. Without Cache-Control: private, no-store (or at minimum private), shared HTTP caches (corporate proxies, CDNs, ISP gateways) may cache the response keyed by the secret token URL. This extends data exposure beyond token rotation/disablement and stores the capability in intermediate infrastructure.
    Fix: Add c.Header("Cache-Control", "private, no-store") before c.JSON in getPublicGarden.
🎯 Correctness — No material issues found

No material issues found

The correctness lens is clean. I verified:

  • Token isolation: gardenColumns (line 13 of internal/store/gardens.go) deliberately omits public_token, and GetGardenByPublicToken uses only that constant — the token cannot leak into the garden payload. The API test explicitly asserts this.
  • Capability model: GET /api/v1/public/gardens/:token is correctly outside requireAuth; the csrfGuard middleware only inspects non-safe methods, so a cookieless GET works as intended.
  • Authz on management: The share-link endpoints are mounted on the gardens group which carries h.requireAuth(), so only authenticated owners can enable/rotate/disable.
  • Payload sanitization: PublicGarden sets OwnerID = 0 and MyRole = "". The frontend gardenSchema requires ownerId: z.number(), so 0 satisfies validation while masking the real owner. myRole is optional() and the domain tag is omitempty, so the empty role is dropped cleanly.
  • Token entropy: newPublicToken() calls randToken(18) → 144 bits, matching the PR claim and producing a 24-char URL-safe segment.
  • Shape parity: GardenFull delegates to the extracted assembleFull, so authenticated and public reads return the identical FullGarden shape.
  • Migration correctness: The partial unique index WHERE public_token IS NOT NULL is valid SQLite and enforces at most one garden per live token.
  • Retry behavior: publicGardenQueryOptions sets retry: false, so a 404 from a rotated/disabled token is treated as final rather than retried.
  • Idempotence: EnablePublicShareLink preserves the existing token when rotate = false; DisablePublicShareLink is idempotent (clearing an already-null token succeeds).

All domain logic, unit conversions (none changed), and boundary behaviors check out.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • web/src/pages/PublicGardenPage.tsx:27-36 — The mount effect manually enumerates seven individual useEditorStore setters to reset transient editor state. It omits objectDragging, so a logged-in user arriving from an active editor gesture (where objectDragging may be true) could find pan/zoom broken on the public canvas because the viewport stands down while that flag is set. More broadly, manually listing fields is fragile: any new transient field added to the editor store will be missed here too.
    Fix: Add a reset() action to useEditorStore that nulls all transient fields (viewport can be kept) and call it in the effect instead of enumerating individual setters.
Performance — No material issues found

No material issues found

The public-share-link change introduces no new performance regressions. The unauthenticated read (PublicGarden) reuses the existing assembleFull helper, which performs the same 4-query pattern (1 garden lookup + 3 child lists) as the authenticated GardenFull endpoint—no N+1 queries, no extra work. The new public_token column is backed by a partial unique index, so token lookups are indexed and fast. Frontend useMemo wrappers on PublicGardenPage prevent avoidable re-allocations of the object/planting/plant arrays passed to GardenCanvas. Nothing in the diff warrants a performance flag.

🧯 Error handling & edge cases — No material issues found

No material issues found

Nothing in my lane warrants a blocking or minor finding. The error paths are handled:

  • Unknown/blank/rotated/disabled tokens return 404 (verified in tests and PublicGarden service).
  • Owner-only endpoints correctly gate on roleOwner via requireGardenRole.
  • The frontend usePublicGarden sets retry: false so a 404 (rotated/disabled link) is not retried.
  • PublicGardenPage guards all data access behind isPending/isError checks.
  • createShareLink intentionally ignores ShouldBindJSON errors for optional bodies; while this could mask a client sending {"rotate": "yes"}, the PR explicitly documents this behavior and it is not an error-handling defect.
opencode/glm-5.2:cloud (opencode) — Minor issues

Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

The finding is confirmed: ListReferencedPlants selects owner_id, custom plants set OwnerID = &ownerID, and PublicGarden only zeroes full.Garden.OwnerID/MyRole, not the referenced plants'.

VERDICT: Minor issues

  • internal/service/public.go:44 redaction is defeated by ListReferencedPlantsPublicGarden deliberately zeroes full.Garden.MyRole and full.Garden.OwnerID (lines 43-44) "to minimize what an anonymous viewer learns," but it returns the result of s.assembleFull, which calls s.store.ListReferencedPlants (internal/service/objects.go). ListReferencedPlants selects plantColumns (internal/store/plants.go:13-14), which includes owner_id, and scanPlant scans it into domain.Plant.OwnerID (internal/store/plants.go:16-25). domain.Plant.OwnerID is *int64 json:"ownerId,omitempty" (internal/domain/domain.go:186), so when non-nil it serializes into the public /full payload. For a custom plant created by the garden owner (CreatePlant sets p.OwnerID = &owner for the actor at internal/service/plants.go:81-82), that ownerId is the garden owner's user id — exactly what the explicit full.Garden.OwnerID = 0 redaction tries to hide. Built-in plants (NULL owner) omit the field, so only custom-plant owners leak. Impact: an anonymous viewer who knows the user-id space can correlate a shared garden back to the owner's account id despite the redaction; the redaction gives a false sense that this is not possible. The existing tests don't catch it — public_test.go only asserts publicToken doesn't leak into the garden object, never that referenced plants don't carry ownerId. Fix: in PublicGarden, also nil out full.Plants[i].OwnerID before returning, or have ListReferencedPlants accept a flag to omit the column for the public path.
🎯 Correctness — No material issues found

I'll review this through the Correctness lens, verifying against the actual code.
I've verified the change through the correctness lens — reading the actual checked-out service, store, API, domain, router, and frontend files rather than trusting the diff in isolation.

Key things I confirmed:

  • gardenColumns (store/gardens.go:13) genuinely excludes public_token, so it can't leak via GetGarden/GardenFull/GetGardenByPublicToken (the latter selects only gardenColumns).
  • assembleFull (service/objects.go:174) is a faithful extraction of the old GardenFull body; both callers pass an authorized *domain.Garden. PublicGarden then zeroes MyRole/OwnerID, and JSON tags (myRole,omitempty + ownerId no-omitempty) produce a payload the frontend gardenSchema still accepts (myRole optional, ownerId: 0 valid).
  • Token math: 18 bytes → base64.RawURLEncoding → 24 chars, 144 bits. Re-derived from first principles; the comment is accurate.
  • EnablePublicShareLink idempotence (rotate=false keeps existing token; rotate=true always mints fresh) and DisablePublicShareLink idempotence (UPDATE NULL→NULL still affects the row, so no spurious ErrNotFound) are correct.
  • Unknown/blank/rotated/disabled tokens all collapse to ErrNotFound → 404: blank is short-circuited in service; NULL public_token doesn't match WHERE public_token = ?; rotated token is gone. Verified against writeServiceError (errors.go:25).
  • SetGardenPublicToken deliberately doesn't bump version — verified this can't cause a spurious 409 on a concurrent garden PATCH (the version guard still matches).
  • Management routes sit under gardens (requireAuth + CSRF); public read sits under public (no auth, GET-only, CSRF allows GET). mustActor is only called on the authed handlers.
  • Migration 0004: SQLite supports partial unique indexes; NULLs are excluded so existing rows don't conflict. The runner embeds and applies it in order.
  • Frontend: PublicGardenPage builds a valid EditorGarden, passes canEdit={false}; GardenCanvas guards every mutation path behind canEdit. The store-reset effect on [token] clears stale focus/selection from a prior editor session. g/$token route has no beforeLoad guard. retry: false is appropriate since 404 is terminal.

I looked hard at the constants (18 bytes/144 bits/24 chars), the version-skipping UPDATE, the omitempty/optional interplay, and the race between concurrent enables — the only race (two owners racing an initial enable) is last-writer-wins with no unique violation and is acceptable for an owner-only, household-scale operation; not a correctness bug.

VERDICT: No material issues found

Through the correctness lens, the change behaves as described. Constants and formulas re-derive correctly, the public/authenticated payloads share an identical shape, the 404-masking is consistent across all dead-token cases, and the frontend read-only path has no editable mutation surface.

🧹 Code cleanliness & maintainability — Minor issues

All five draft findings checked against the actual source. Finding 4 (the ShouldBindJSON swallow comment) is dropped: the function's doc comment at internal/api/public.go:37-39 already explicitly states "The body is optional — a missing/malformed one just means enable-without-rotate" — exactly the intent the finding asks to be documented, so there is nothing missing. The other four survive verification.

VERDICT: Minor issues

The PR is well-structured and largely follows existing patterns. The assembleFull extraction in internal/service/objects.go is a clean, well-documented refactor; the new handlers mirror the existing parseIDParam/mustActor/writeServiceError shape faithfully; randToken is a sensible generalization of the old newSessionToken. Findings through the cleanliness/maintainability lens:

  • web/src/pages/PublicGardenPage.tsx:27 duplicates the editor-store reset sequence from web/src/pages/GardenEditorPage.tsx:76-92. Both call setFocusedObject(null)select(null)selectPlanting(null)setArmedPlant(null)/setArmedKind(null)setLiveObject(null)setLivePlanting(null) (the editor page's clear() omits setFocusedObject, which it instead handles separately around focus). Extract a single resetTransientEditorState() helper on the store so the two callers can't drift and the public page stops reaching across the store's surface for seven setters. Verified by reading both files.

  • web/src/pages/PublicGardenPage.tsx:38 and :52 duplicate GardenEditorPage.tsx:55-60 and :219-226 — the objects/plantings/plants/plantsById memo block (functionally identical; the editor page just routes through intermediate serverObjects/serverPlantings locals) and the five-field EditorGarden copy from full.data.garden (line-for-line identical). This is the canonical "build editor view-model from FullGarden" transform; both pages should share a useEditorViewModel(full) hook or toEditorGarden(full) helper. Verified by reading both files.

  • internal/api/public.go:67 returns gin.H{"enabled": false} while getShareLink/createShareLink return the typed *PublicShareLink. The disable response shape is now hand-built in the handler and will silently fall out of sync with PublicShareLink if it gains a field. The service's DisablePublicShareLink returns only error, so either have it return the resulting disabled *PublicShareLink, or have the handler return &PublicShareLink{Enabled: false}. Verified by reading the file and the service signature.

  • web/src/lib/shares.ts:78 shareLinkSchema's token: z.string().optional() imposes no format/length invariant, while the backend generates a 24-char URL-safe token (newPublicTokenrandToken(18) → base64-rawurl) and the rest of the data layer (e.g. shareRoleSchema as z.enum) encodes its invariants in zod. A .min(1).regex(/^[A-Za-z0-9_-]+$/) would make the contract explicit and catch a backend regression. Verified by reading the file and internal/service/service.go.

Performance — No material issues found

No material issues found through the Performance lens.

The change is performance-clean in my lane:

  • assembleFull is a faithful extraction of the existing GardenFull body — same three store calls, same map-lookup DerivedCount fill (no new N+1). The public path runs the identical query set as the authenticated path, no extra work.
  • GetGardenByPublicToken queries on the unique-indexed public_token column (migration 0004's idx_gardens_public_token), so the unauthenticated lookup is a single indexed point read — no scan, no missing pagination.
  • newPublicToken allocates 18 bytes vs. the session's 32 — trivially less, fine.
  • No new hot loops, no unbounded growth, no missing limits introduced. The public endpoint has no rate limiting, but that's an availability/security concern, not a per-request efficiency regression — outside my lane.
🧯 Error handling & edge cases⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run context deadline exceeded

opencode/kimi-k2.6:cloud (opencode) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Blocking issues found

Blocking issues found

  • internal/service/public.go:43-44PublicGarden clears OwnerID on the top-level garden but not on nested Plants. assembleFull loads referenced plants via ListReferencedPlants, and each domain.Plant carries an OwnerID *int64 (the user ID of the plant's creator). For custom plants this leaks the owner's user ID — exactly the kind of identity disclosure the PR says it prevents by zeroing Garden.OwnerID. Worse, if a co-editor created a planting with their own custom plant, that co-editor's user ID is leaked too.

    Fix: After assembleFull, iterate over full.Plants and nil out OwnerID so the field is omitted from JSON:

    for i := range full.Plants {
        full.Plants[i].OwnerID = nil
    }
    

    Also add assertions in TestPublicShareLink and TestPublicShareLinkAPI verifying that no ownerId appears in plants/objects of the public payload.

🎯 Correctness — No material issues found

Verdict: No material issues found

Review notes (verified):

  • Token entropy: randToken(18) → 18 bytes = 144 bits, base64url raw (no padding) → 24 characters. This matches the PR description and is a reasonable capability-token size (comparable to e.g. Dropbox / Google Docs share tokens).
  • Public GET semantics: PublicGarden trims and rejects empty tokens, returns domain.ErrNotFound for unknown/rotated/disabled tokens, which writeServiceError maps to 404. Never redirects.
  • Token isolation from normal garden payload: gardenColumns does not include public_token; GetGardenByPublicToken selects only gardenColumns, so the token cannot leak through the normal garden response. Confirmed by reading internal/store/gardens.go:13.
  • Minimization of leaked info: full.Garden.MyRole = "" and full.Garden.OwnerID = 0 are correctly set before returning the public payload. The frontend gardenSchema parses ownerId as z.number(), so 0 parses fine.
  • ACL on management endpoints: All three management handlers (getShareLink, createShareLink, deleteShareLink) call mustActor(c), which requires an authenticated session. The service layer further enforces roleOwner via requireGardenRole.
  • CSRF protection: v1.Use(h.csrfGuard()) applies to all state-changing routes under /api/v1, including the new POST/DELETE share-link endpoints. Safe methods (GET) are explicitly exempted, which is correct for the public read endpoint.
  • Route collision: g/$token does not collide with gardens/$gardenId; the router distinguishes them correctly.
  • Frontend useEffect cleanup: The PublicGardenPage effect resets transient editor state on mount with token as the dependency, which correctly re-runs when the URL token changes.

Nothing in the correctness lens requires a finding.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

I verified the PR through the code-cleanliness lens and found nothing material to report.

What I checked:

  • Backend structure: internal/api/public.go and internal/service/public.go follow the existing thin-handler / thick-service pattern consistently. The linkState helper is appropriately scoped and simple.
  • Refactoring quality: Extracting assembleFull in internal/service/objects.go cleanly eliminates duplication between authenticated GardenFull and the new public read — this is exactly the kind of DRY improvement this lens rewards.
  • Naming: PublicGarden, EnablePublicShareLink, PublicLinkSection, usePublicGarden, etc. are all descriptive and consistent with surrounding code.
  • Frontend patterns: The new publicGarden.ts hook follows the identical queryOptions + useQuery pattern used by gardens.ts and objects.ts. PublicGardenPage.tsx is appropriately sized and reuses existing GardenCanvas, toEditorObject, and toEditorPlanting helpers rather than duplicating transformation logic.
  • Store layer: GetGardenByPublicToken, GetGardenPublicToken, and SetGardenPublicToken in internal/store/gardens.go match the existing store patterns (error wrapping, RowsAffected check, sql.NullString for nullable text, var val any for driver arguments — all idiomatic for Go 1.26).
  • Router: Adding publicGardenRoute to router.tsx follows the exact createRoute pattern of its siblings; the inline comment explaining the missing beforeLoad guard is appropriate.

The change is well-structured, follows established patterns, and introduces no readability or maintainability regressions.

Performance — No material issues found

No material issues found

I scrutinized this PR through the performance lens and found nothing material to report. Here is what I verified:

Backend queries

  • internal/service/public.go:28-46PublicGarden does a single indexed lookup (GetGardenByPublicToken) followed by assembleFull. The latter runs the same three queries (ListObjectsForGarden, ListActivePlantingsForGarden, ListReferencedPlants) that GardenFull already ran before this PR, so there is no new N+1 or extra query overhead.
  • internal/store/gardens.go:162-172GetGardenByPublicToken filters by the unique partial index idx_gardens_public_token; the planner will use the index. No table-scan risk.
  • internal/store/gardens.go:196-214SetGardenPublicToken is a single-row UPDATE by primary key. Correct and cheap.

Frontend

  • web/src/pages/PublicGardenPage.tsx:38-41 — The four useMemo hooks around objects, plantings, plants, and plantsById are reasonable and prevent redundant re-creation on stable data.
  • web/src/lib/publicGarden.tsstaleTime: 30_000 and retry: false are sensible defaults for a public read endpoint.

The change is a clean refactor with no efficiency regressions, unbounded growth, or hot-path allocations introduced.

🧯 Error handling & edge cases — Minor issues

Minor issues

  • web/src/components/gardens/ShareGardenModal.tsx:167 — Missing cleanup for the setTimeout in copy(). If PublicLinkSection unmounts before the 1500 ms timeout fires (e.g., user closes the Share dialog right after clicking Copy), React will warn about a state update on an unmounted component. Multiple rapid clicks on Copy also create overlapping timeouts that can prematurely clear the copied state.
    Fix: Track the timeout ID in a ref and clear it both before starting a new one and in a useEffect cleanup:
const timeoutRef = useRef<number | null>(null)
useEffect(() => () => { if (timeoutRef.current) window.clearTimeout(timeoutRef.current) }, [])

async function copy() {
  if (!url) return
  try {
    await navigator.clipboard.writeText(url)
    setCopied(true)
    if (timeoutRef.current) window.clearTimeout(timeoutRef.current)
    timeoutRef.current = window.setTimeout(() => setCopied(false), 1500)
  } catch { /* … */ }
}
  • internal/api/public.go:48–49createShareLink intentionally swallows ShouldBindJSON errors so a malformed body defaults to rotate=false. While documented, a client sending \"rotate\": \"yes\" (string instead of bool) silently gets a different result than requested with no feedback. This is a design choice, not a bug, but worth noting as an unhandled edge case in the API contract.

Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 5 models **Verdict: Blocking issues found** · 11 findings (4 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟠 | Manual store reset omits objectDragging and is fragile to future store additions | `web/src/pages/PublicGardenPage.tsx:27` | 4/5 | maintainability | | 🟠 | Plant.OwnerID leaks in public payload despite Garden.OwnerID sanitization | `internal/service/public.go:43` | 3/5 | security | | 🟠 | Public read scrubs garden.OwnerID but plants still leak the owner's user id via Plant.OwnerID | `internal/service/public.go:36` | 2/5 | correctness, security | | 🟡 | createShareLink silently discards malformed JSON body instead of returning 400 | `internal/api/public.go:48` | 2/5 | error-handling | <details><summary>7 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | Public garden endpoint lacks cache-control, allowing shared proxy caching of capability-URL responses | `internal/api/public.go:13` | kimi-k2.6:cloud | security | | 🟠 | Editor view-model memos + EditorGarden mapping duplicated from GardenEditorPage; extract toEditorGarden/useEditorViewModel | `web/src/pages/PublicGardenPage.tsx:38` | opencode/glm-5.2:cloud | maintainability | | 🟡 | Public garden read has no HTTP cache headers despite being the many-viewer hot path (4 DB queries per load) | `internal/api/public.go:19` | glm-5.2:cloud | performance | | 🟡 | deleteShareLink hand-builds gin.H instead of returning typed PublicShareLink; response shape can drift | `internal/api/public.go:67` | opencode/glm-5.2:cloud | maintainability | | 🟡 | Missing setTimeout cleanup in copy() can set state on unmounted component | `web/src/components/gardens/ShareGardenModal.tsx:167` | opencode/kimi-k2.6:cloud | error-handling | | 🟡 | shareLinkSchema.token has no format/length invariant; backend regression would pass silently | `web/src/lib/shares.ts:78` | opencode/glm-5.2:cloud | maintainability | | ⚪ | Second ad-hoc mutation-error-to-state helper added alongside existing onMutationError in the same file | `web/src/components/gardens/ShareGardenModal.tsx:157` | claude-code/sonnet | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> These supporting claims all check out and are consistent with the diff. The one finding is solidly confirmed by the actual source. My review is complete. **VERDICT: Minor issues** - `internal/service/public.go:37-38` (`PublicGarden`) — the anonymous-viewer redaction is incomplete. It zeroes `full.Garden.MyRole` and `full.Garden.OwnerID`, but `full.Plants` (from `ListReferencedPlants`, `internal/store/plants.go:31-57`) is passed through unredacted. `plantColumns`/`scanPlant` (`internal/store/plants.go:13-25`) populate `domain.Plant.OwnerID` (`internal/domain/domain.go:186`, `*int64`, `json:"ownerId,omitempty"`, nil only for built-ins) directly from `plants.owner_id`. Any custom (non-built-in) plant referenced by an active planting in the garden — created by the owner or any editor/collaborator — leaks that user's real numeric user ID to an anonymous visitor holding the link. This contradicts the function's own stated intent ("minimize what an anonymous viewer learns... don't expose the owner's user id") and is inconsistent within the same function. Confirmed the shipped test (`internal/service/public_test.go`) seeds a plant via `seedOwnPlant(t, s, owner, 10)` (owned by the real `owner` user) and asserts it appears in `full.Plants` (`len(full.Plants) != 1` check) without ever checking/scrubbing `OwnerID` — the leak is present in the shipped fixture, just not asserted on. Fix: zero out (or omit) `OwnerID` on each entry in `full.Plants` inside `PublicGarden`, same as done for the garden itself. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> Both helper functions exist and are used identically to how `GardenEditorPage.tsx` uses them, so no signature mismatch there either. VERDICT: No material issues found I reviewed this PR through the correctness lens — logic bugs and semantic/domain correctness (constants, formulas, thresholds). Things I specifically verified against the checked-out code: - **Token entropy math**: `newPublicToken` uses `randToken(18)` → 18×8 = 144 bits, matching the PR's claim, and base64-raw-url-encoding 18 bytes yields a 24-char string, matching the doc comment (`internal/service/service.go`). - **`public_token` never leaks into normal reads**: confirmed `gardenColumns` (`internal/store/gardens.go:13`) does not include `public_token`, so `GetGarden`/`GardenFull`/`assembleFull` can't select it. - **`assembleFull` refactor** (`internal/service/objects.go:161-193`): all three store calls now key off `g.ID` instead of the old `gardenID` parameter — same value, no behavior change; confirmed `GardenFull` still authorizes via `requireGardenRole(..., roleViewer)` before delegating. - **`PublicGarden` masking of `MyRole`/`OwnerID`** (`internal/service/public.go:35-38`): `GetGardenByPublicToken` never sets `MyRole` (only `requireGardenRole` does, via `effectiveGardenRole`), so the explicit `full.Garden.MyRole = ""` is redundant-but-harmless; `OwnerID = 0` correctly masks the real owner ID. `Garden.OwnerID` json tag has no `omitempty` so it always serializes as `0`, `MyRole` has `omitempty` so it's dropped entirely — both are consistent with what the frontend `gardenSchema` expects (`ownerId: z.number()` required, `myRole` optional). - **Enable/rotate/disable idempotency logic** (`internal/service/public.go:71-93`): `token == nil || rotate` correctly captures "not yet enabled" vs "force new token"; `SetGardenPublicToken(ctx, gardenID, nil)` correctly clears via a nil `any` → SQL NULL. - **Role-check consistency**: owner-only endpoints route through the same `requireGardenRole(..., roleOwner)` used elsewhere (`shares.go`), producing the same `ErrForbidden`-for-editor / `ErrNotFound`-for-stranger split the tests assert. - **Route wiring**: `getShareLink`/`createShareLink`/`deleteShareLink`/`getPublicGarden`, `parseIDParam`, `mustActor`, `writeServiceError` all exist with matching signatures; the public route sits inside the `v1` group but `csrfGuard` exempts GET/HEAD/OPTIONS, so the unauthenticated read isn't accidentally blocked. - **Frontend**: `PublicGardenPage` builds `EditorGarden` and calls `GardenCanvas` the same way `GardenEditorPage` does; `toEditorObject`/`toEditorPlanting` signatures match. Nothing here is logically wrong. The one soft observation — `SetGardenPublicToken` doesn't handle a UNIQUE-constraint collision on the vanishingly-unlikely case of a token collision — isn't a real correctness bug at 144 bits of entropy, so I'm not reporting it as a finding. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## VERDICT: Minor issues - **`web/src/pages/PublicGardenPage.tsx:27-36`** and **`web/src/pages/GardenEditorPage.tsx:76-92`**: both effects reset the same six transient `useEditorStore` fields (`select`, `selectPlanting`, `setArmedKind`, `setArmedPlant`, `setLiveObject`, `setLivePlanting`, plus `setFocusedObject`) as independently-maintained call sites, confirmed by reading both files — `web/src/editor/store.ts:54-81` has no shared `resetTransient()`-style action. The two lists already differ in order/grouping (and `GardenEditorPage` additionally runs the reset on unmount too), which is the kind of drift that creeps in when the same logic is copy-pasted at two sites. Low-churn fix: hoist into a single store action both pages call. - **`web/src/components/gardens/ShareGardenModal.tsx:58`** vs **`:157-160`**: confirmed — the file defines `onMutationError` (used via `mutate(..., { onError })` at lines 108/121) and a separately-invented `run()` helper (wraps `mutateAsync` in `.catch()`, used at lines 185/208/217), both doing nothing but `setError(errorMessage(err, fallback))`. Not a hard blocker given the different mutate styles at each call site, but it's a missed chance to reuse one pattern instead of adding a second. Everything else in the draft (Go handlers/service/store following existing conventions, `assembleFull` extraction being an appropriately-scoped dedup) is general commentary rather than a specific verifiable claim, and nothing in it contradicts the code read. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Default staleTime is 30s app-wide, so `shareLinkQueryOptions` inherits sane caching. No refetch storms. I reviewed the diff end-to-end through the performance lens and verified the query paths directly. **VERDICT: No material issues found** - `assembleFull` (`internal/service/objects.go`) is a straight extraction of the existing 3-query load (`ListObjectsForGarden`, `ListActivePlantingsForGarden`, `ListReferencedPlants`) — verified each is a single indexed query (`idx_garden_objects_garden`, `idx_plantings_object`/`idx_plantings_plant`), no N+1 introduced, no behavior change vs. the pre-existing `GardenFull`. - `GetGardenByPublicToken` (`internal/store/gardens.go:164`) is a single-row lookup backed by the new partial unique index `idx_gardens_public_token` (`0004_garden_public_token.sql:7`). Equality against a non-NULL bound parameter is eligible for SQLite's partial-index optimization, so this is an indexed lookup, not a scan. - `GetGardenPublicToken`/`SetGardenPublicToken` are simple single-row reads/updates on the PK; `SetGardenPublicToken` deliberately skips bumping `version`, which avoids spurious extra work/conflicts for concurrent editors but has no query-count impact either way. - Frontend: `usePublicGarden` sets `staleTime: 30_000` and `retry: false` (`web/src/lib/publicGarden.ts:15`), and `useShareLink` inherits the app-wide `staleTime: 30_000` default (`web/src/lib/queryClient.ts:8`) — no polling or refetch storms. `PublicGardenPage` memoizes the objects/plantings/plants derivations off `full.data`, so no per-render recomputation of the full lists. - No loops, no per-item queries, no unbounded new payload shape beyond what the already-existing authenticated `/full` endpoint returns. One observation that's adjacent to performance but really a security/access-control concern (removing the auth barrier means this bounded-but-potentially-large read is now reachable by anonymous, unrate-limited traffic) — noting it but not flagging it here since the app has no rate limiting anywhere today (this diff doesn't regress that posture, it's consistent with the rest of the API), and the auth-exposure angle itself belongs to the security lens, not mine. Outside my lens: none worth flagging beyond the note above. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Good, confirms `public_token` is indeed excluded from `gardenColumns`, backing the "never leaks" claim in the draft's clean-list. **VERDICT: Minor issues** - `internal/api/public.go:48` (`createShareLink`) — `_ = c.ShouldBindJSON(&req)` discards the bind error entirely, not just for a missing body. If a client sends a **malformed** JSON body (e.g. a syntax error from a client bug) rather than no body at all, `ShouldBindJSON` fails, `req` is left at its zero value, and the handler silently proceeds as `rotate=false` and returns `200 OK`. The doc comment frames this as "missing/malformed just means enable-without-rotate," so it's an intentional design choice, but it conflates "no opinion supplied" (fine to default) with "client tried to send something and it was broken" (every other body-parsing endpoint in this codebase — `auth.go:39/60`, `gardens.go:52/83`, `shares.go:40/62`, `plants.go:84/102`, `plantings.go:83/101`, `objects.go:117/135` — checks the error and returns 400). Practical impact is low, but a caller intending to rotate with a serialization bug gets a silent no-op instead of an error signal. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> Confirmed. The finding is verified. **VERDICT: Minor issues** Through the security lens I verified the core design: the 18-byte (144-bit) `crypto/rand` token is the capability, the management endpoints sit behind `requireAuth` + `requireGardenRole(roleOwner)` + the `csrfGuard` (which rejects cross-origin state changes when `PANSY_BASE_URL` is set), `public_token` is deliberately excluded from `gardenColumns` so it can't leak via the normal garden payload, and unknown/rotated/disabled/blank tokens are masked as 404 via `domain.ErrNotFound`. The `GardenFull`/`assembleFull` split is shape-identical. SQL is parameterized (`?` placeholders) — no injection. The unauthenticated `GET /api/v1/public/gardens/:token` is correctly GET-only. - **`internal/service/public.go:43-44` (info disclosure, verified)** — `PublicGarden` zeroes `full.Garden.MyRole` and `full.Garden.OwnerID` "to minimize what an anonymous viewer learns," but the `Plants` slice returned by `assembleFull` (`internal/service/objects.go:196`) still carries each `domain.Plant.OwnerID` (`internal/domain/domain.go:186`, `json:"ownerId,omitempty"`). `ListReferencedPlants` selects `plantColumns` including `owner_id` (`internal/store/plants.go:33`). If the garden's owner has referenced any *custom* (non-built-in) plant — `owner_id` is their own user id — an anonymous viewer learns the owner's account id through `plants[].ownerId`, defeating the `OwnerID` masking the PR explicitly set out to do. Built-in plants (`owner_id IS NULL`, omitted from JSON) don't leak, so the exposure is conditional on custom-plant use, but it's a real, inconsistent identifier disclosure on the unauthenticated read path. Suggested fix: in `PublicGarden`, zero `p.OwnerID` for each plant in `full.Plants` (mirroring the `OwnerID=0` treatment already applied to the garden), or have `assembleFull` take a `public` flag that strips owner identifiers from plants too. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Confirmed: `domain.Plant.OwnerID *int64` (json `ownerId,omitempty`) is populated by `ListReferencedPlants` (which selects `owner_id` via `plantColumns` and scans into `&p.OwnerID`), and `CreatePlant` sets `p.OwnerID = &actorID`. In the test's `seedOwnPlant(t, s, owner, 10)` case the plant's `OwnerID` equals the garden owner's id, while `PublicGarden` only zeroes `full.Garden.OwnerID`/`MyRole` and leaves `full.Plants[*].OwnerID` intact, so the owner's user id is re-exposed via the plants array. The finding survives. VERDICT: Minor issues - `internal/service/public.go:36-44` — The public read scrubs `full.Garden.OwnerID` and `MyRole`, but `full.Plants` still carries each plant's `OwnerID` (`domain.Plant.OwnerID *int64`, json `ownerId,omitempty`, per `internal/domain/domain.go:186`). `ListReferencedPlants` selects `owner_id` (`internal/store/plants.go:13` / `plantColumns`) and scans it into `p.OwnerID` (`internal/store/plants.go:19`), so the field is populated. For a garden whose owner created the referenced plants (the common case — e.g. `seedOwnPlant` in `internal/service/plantings_test.go:24`, where `CreatePlant` sets `p.OwnerID = &actorID`, `internal/service/plants.go:81-82`), `full.Plants[*].ownerId` equals the garden owner's user id, so the stated goal ("don't expose the owner's user id") is only half-met: the owner ID is removed from the `garden` object but re-leaks through the `plants` array. This is a semantic-correctness gap (behavior doesn't match the documented intent). Suggested fix: zero/omit `OwnerID` on each plant in the `PublicGarden` path (or drop `ownerId` from the public plants payload), matching the `MyRole`/`OwnerID` scrubbing already done on the garden. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## Verified review **Verdict: Minor issues** - `web/src/pages/PublicGardenPage.tsx:27-36` — The mount effect imperatively resets seven fields on the shared singleton `useEditorStore`, duplicating the "clear transient editor state" logic that `GardenEditorPage` performs in its own mount effect (`web/src/pages/GardenEditorPage.tsx:76-92`, where the `clear()` closure calls `select(null)`/`selectPlanting(null)`/`setArmedKind(null)`/`setArmedPlant(null)`/`setLiveObject(null)`/`setLivePlanting(null)`, with `setFocusedObject` handled separately). Both pages share the same store. Verified against `web/src/editor/store.ts` (lines 14-81): the store exposes only individual setters (`select`, `selectPlanting`, `setFocusedObject`, `setArmedPlant`, `setArmedKind`, `setLiveObject`, `setLivePlanting`) with no aggregate `reset()`/`resetTransient()` action, so this is copy-paste that must be kept in sync by hand whenever a new transient field is added. Maintainability hazard; should be a single shared store action called by both pages. - `internal/api/public.go:48` — `createShareLink` discards the bind error with `_ = c.ShouldBindJSON(&req)`. This is intentional per the function-level comment ("a missing/malformed one just means enable-without-rotate") and matches the idempotent-enable semantics, so it's defensible. Only a trivial readability nit: a one-line comment near the `_ =` would make the deliberate discard obvious to readers who skip the doc comment. No change required. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> Confirmed. `internal/api/public.go:19` returns via `c.JSON` with no cache headers; `assembleFull` (objects.go:174-186) runs 3 indexed queries plus the `GetGardenByPublicToken` lookup = 4 DB queries per load; only `spa.go` sets `Cache-Control` in the API package; `web/src/lib/publicGarden.ts:16` has `staleTime: 30_000`. The finding survives. ## Verdict: Minor issues - **`internal/api/public.go:19` — public garden read sets no HTTP cache headers, yet is the one endpoint explicitly designed for many anonymous viewers.** Each page load by each visitor runs 4 SQL queries (`GetGardenByPublicToken` plus the three `assembleFull` queries in `internal/service/objects.go:175-183`: `ListObjectsForGarden`, `ListActivePlantingsForGarden`, `ListReferencedPlants`). The authenticated `GardenFull` path is single-user-per-garden so this didn't matter before, but a share link is specifically meant to be opened broadly and repeatedly. There's no `Cache-Control`/`ETag` on the response, so browsers/intermediaries re-hit the DB on every navigation, and react-query's `staleTime: 30_000` (`web/src/lib/publicGarden.ts:16`) only helps within a single tab. The response is read-only and changes only on owner edits, so a short `Cache-Control: public, max-age=…` (or an ETag keyed on `garden.updated_at`/`version`) would cut repeated DB load for the shared-link hot path. Confirmed by grep that only `spa.go` sets cache headers in the API package. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict: No material issues found** I reviewed the unhappy paths the diff introduces, verified against the checked-out code: - **`createShareLink` swallowing `c.ShouldBindJSON` error** (`internal/api/public.go:44`): `_ = c.ShouldBindJSON(&req)` is intentional and safe — a malformed/missing body leaves `req.Rotate` at its zero value `false`, which maps to the documented "enable without rotate" path. Not a bug. - **`PublicGarden` masking fields** (`internal/service/public.go:23-34`): blank token → `ErrNotFound`; `GetGardenByPublicToken` maps `sql.ErrNoRows` → `ErrNotFound`; other store errors are wrapped and surface via the mapper. `MyRole=""` is omitted by the struct's `omitempty`, so the frontend's `gardenRoleSchema.optional()` parses; `OwnerID=0` is a valid number for the required `ownerId` field. No parse/edge failure. Verified `gardenSchema` in `web/src/lib/gardens.ts:14` and `domain.Garden` tags in `internal/domain/domain.go:122`. - **`SetGardenPublicToken` nil path** (`internal/store/gardens.go:202`): `token==nil` → `val` stays the untyped `nil` → `NULL`. `RowsAffected()==0` → `ErrNotFound`. Correct disable/idempotent behavior. - **Frontend `PublicGardenPage`** guards `full.isPending` and `full.isError` before touching `full.data`; the `?.map(...) ?? []` memos are null-safe during pending. The clipboard `catch {}` in `PublicLinkSection` is a deliberate silent fallback with a comment. - **`run` helper** (`ShareGardenModal.tsx`) always attaches `.catch`, so no unhandled promise rejection; `setError(null)` before the async op is fine. One trivial edge case worth noting (low confidence, not blocking): - `internal/store/gardens.go:206` / `internal/service/public.go:62`: `newPublicToken()` generates 144 random bits and writes them under a `UNIQUE` index (`0004_garden_public_token.sql`). On the astronomically unlikely collision, the DB returns a unique-constraint violation that `SetGardenPublicToken` wraps but does not map to a sentinel, so `writeServiceError` (`internal/api/errors.go:55`) falls through to `500 INTERNAL`. The probability is negligible at household scale, so this is not material, but a retry-on-conflict or a mapped "TOKEN_COLLISION" would be the tidy fix if you ever care. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> **Verdict: Minor issues** - **`internal/api/public.go:13`** — `getPublicGarden` returns sensitive garden data for a capability-URL but sets no `Cache-Control` header. Without `Cache-Control: private, no-store` (or at minimum `private`), shared HTTP caches (corporate proxies, CDNs, ISP gateways) may cache the response keyed by the secret token URL. This extends data exposure beyond token rotation/disablement and stores the capability in intermediate infrastructure. **Fix:** Add `c.Header("Cache-Control", "private, no-store")` before `c.JSON` in `getPublicGarden`. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** The correctness lens is clean. I verified: - **Token isolation**: `gardenColumns` (line 13 of `internal/store/gardens.go`) deliberately omits `public_token`, and `GetGardenByPublicToken` uses only that constant — the token cannot leak into the garden payload. The API test explicitly asserts this. - **Capability model**: `GET /api/v1/public/gardens/:token` is correctly outside `requireAuth`; the `csrfGuard` middleware only inspects non-safe methods, so a cookieless GET works as intended. - **Authz on management**: The `share-link` endpoints are mounted on the `gardens` group which carries `h.requireAuth()`, so only authenticated owners can enable/rotate/disable. - **Payload sanitization**: `PublicGarden` sets `OwnerID = 0` and `MyRole = ""`. The frontend `gardenSchema` requires `ownerId: z.number()`, so `0` satisfies validation while masking the real owner. `myRole` is `optional()` and the domain tag is `omitempty`, so the empty role is dropped cleanly. - **Token entropy**: `newPublicToken()` calls `randToken(18)` → 144 bits, matching the PR claim and producing a 24-char URL-safe segment. - **Shape parity**: `GardenFull` delegates to the extracted `assembleFull`, so authenticated and public reads return the identical `FullGarden` shape. - **Migration correctness**: The partial unique index `WHERE public_token IS NOT NULL` is valid SQLite and enforces at most one garden per live token. - **Retry behavior**: `publicGardenQueryOptions` sets `retry: false`, so a 404 from a rotated/disabled token is treated as final rather than retried. - **Idempotence**: `EnablePublicShareLink` preserves the existing token when `rotate = false`; `DisablePublicShareLink` is idempotent (clearing an already-null token succeeds). All domain logic, unit conversions (none changed), and boundary behaviors check out. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`web/src/pages/PublicGardenPage.tsx:27-36`** — The mount effect manually enumerates seven individual `useEditorStore` setters to reset transient editor state. It omits `objectDragging`, so a logged-in user arriving from an active editor gesture (where `objectDragging` may be `true`) could find pan/zoom broken on the public canvas because the viewport stands down while that flag is set. More broadly, manually listing fields is fragile: any new transient field added to the editor store will be missed here too. **Fix:** Add a `reset()` action to `useEditorStore` that nulls all transient fields (viewport can be kept) and call it in the effect instead of enumerating individual setters. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** The public-share-link change introduces no new performance regressions. The unauthenticated read (`PublicGarden`) reuses the existing `assembleFull` helper, which performs the same 4-query pattern (1 garden lookup + 3 child lists) as the authenticated `GardenFull` endpoint—no N+1 queries, no extra work. The new `public_token` column is backed by a partial unique index, so token lookups are indexed and fast. Frontend `useMemo` wrappers on `PublicGardenPage` prevent avoidable re-allocations of the object/planting/plant arrays passed to `GardenCanvas`. Nothing in the diff warrants a performance flag. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found Nothing in my lane warrants a blocking or minor finding. The error paths are handled: - Unknown/blank/rotated/disabled tokens return 404 (verified in tests and `PublicGarden` service). - Owner-only endpoints correctly gate on `roleOwner` via `requireGardenRole`. - The frontend `usePublicGarden` sets `retry: false` so a 404 (rotated/disabled link) is not retried. - `PublicGardenPage` guards all data access behind `isPending`/`isError` checks. - `createShareLink` intentionally ignores `ShouldBindJSON` errors for optional bodies; while this could mask a client sending `{"rotate": "yes"}`, the PR explicitly documents this behavior and it is not an error-handling defect. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> The finding is confirmed: `ListReferencedPlants` selects `owner_id`, custom plants set `OwnerID = &ownerID`, and `PublicGarden` only zeroes `full.Garden.OwnerID`/`MyRole`, not the referenced plants'. **VERDICT: Minor issues** - **`internal/service/public.go:44` redaction is defeated by `ListReferencedPlants`** — `PublicGarden` deliberately zeroes `full.Garden.MyRole` and `full.Garden.OwnerID` (lines 43-44) "to minimize what an anonymous viewer learns," but it returns the result of `s.assembleFull`, which calls `s.store.ListReferencedPlants` (`internal/service/objects.go`). `ListReferencedPlants` selects `plantColumns` (`internal/store/plants.go:13-14`), which includes `owner_id`, and `scanPlant` scans it into `domain.Plant.OwnerID` (`internal/store/plants.go:16-25`). `domain.Plant.OwnerID` is `*int64 json:"ownerId,omitempty"` (`internal/domain/domain.go:186`), so when non-nil it serializes into the public `/full` payload. For a custom plant created by the garden owner (`CreatePlant` sets `p.OwnerID = &owner` for the actor at `internal/service/plants.go:81-82`), that `ownerId` is the garden owner's user id — exactly what the explicit `full.Garden.OwnerID = 0` redaction tries to hide. Built-in plants (NULL owner) omit the field, so only custom-plant owners leak. Impact: an anonymous viewer who knows the user-id space can correlate a shared garden back to the owner's account id despite the redaction; the redaction gives a false sense that this is not possible. The existing tests don't catch it — `public_test.go` only asserts `publicToken` doesn't leak into the garden object, never that referenced plants don't carry `ownerId`. Fix: in `PublicGarden`, also nil out `full.Plants[i].OwnerID` before returning, or have `ListReferencedPlants` accept a flag to omit the column for the public path. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I'll review this through the Correctness lens, verifying against the actual code. I've verified the change through the correctness lens — reading the actual checked-out service, store, API, domain, router, and frontend files rather than trusting the diff in isolation. Key things I confirmed: - `gardenColumns` (store/gardens.go:13) genuinely excludes `public_token`, so it can't leak via `GetGarden`/`GardenFull`/`GetGardenByPublicToken` (the latter selects only `gardenColumns`). - `assembleFull` (service/objects.go:174) is a faithful extraction of the old `GardenFull` body; both callers pass an authorized `*domain.Garden`. `PublicGarden` then zeroes `MyRole`/`OwnerID`, and JSON tags (`myRole,omitempty` + `ownerId` no-omitempty) produce a payload the frontend `gardenSchema` still accepts (`myRole` optional, `ownerId: 0` valid). - Token math: 18 bytes → `base64.RawURLEncoding` → 24 chars, 144 bits. Re-derived from first principles; the comment is accurate. - `EnablePublicShareLink` idempotence (rotate=false keeps existing token; rotate=true always mints fresh) and `DisablePublicShareLink` idempotence (UPDATE NULL→NULL still affects the row, so no spurious ErrNotFound) are correct. - Unknown/blank/rotated/disabled tokens all collapse to `ErrNotFound` → 404: blank is short-circuited in service; NULL `public_token` doesn't match `WHERE public_token = ?`; rotated token is gone. Verified against `writeServiceError` (errors.go:25). - `SetGardenPublicToken` deliberately doesn't bump `version` — verified this can't cause a spurious 409 on a concurrent garden PATCH (the version guard still matches). - Management routes sit under `gardens` (requireAuth + CSRF); public read sits under `public` (no auth, GET-only, CSRF allows GET). `mustActor` is only called on the authed handlers. - Migration 0004: SQLite supports partial unique indexes; NULLs are excluded so existing rows don't conflict. The runner embeds and applies it in order. - Frontend: `PublicGardenPage` builds a valid `EditorGarden`, passes `canEdit={false}`; `GardenCanvas` guards every mutation path behind `canEdit`. The store-reset effect on `[token]` clears stale focus/selection from a prior editor session. `g/$token` route has no `beforeLoad` guard. `retry: false` is appropriate since 404 is terminal. I looked hard at the constants (18 bytes/144 bits/24 chars), the version-skipping `UPDATE`, the omitempty/optional interplay, and the race between concurrent enables — the only race (two owners racing an initial enable) is last-writer-wins with no unique violation and is acceptable for an owner-only, household-scale operation; not a correctness bug. ## VERDICT: No material issues found Through the correctness lens, the change behaves as described. Constants and formulas re-derive correctly, the public/authenticated payloads share an identical shape, the 404-masking is consistent across all dead-token cases, and the frontend read-only path has no editable mutation surface. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All five draft findings checked against the actual source. Finding 4 (the `ShouldBindJSON` swallow comment) is dropped: the function's doc comment at `internal/api/public.go:37-39` already explicitly states "The body is optional — a missing/malformed one just means enable-without-rotate" — exactly the intent the finding asks to be documented, so there is nothing missing. The other four survive verification. ## VERDICT: Minor issues The PR is well-structured and largely follows existing patterns. The `assembleFull` extraction in `internal/service/objects.go` is a clean, well-documented refactor; the new handlers mirror the existing `parseIDParam`/`mustActor`/`writeServiceError` shape faithfully; `randToken` is a sensible generalization of the old `newSessionToken`. Findings through the cleanliness/maintainability lens: - **`web/src/pages/PublicGardenPage.tsx:27` duplicates the editor-store reset sequence from `web/src/pages/GardenEditorPage.tsx:76-92`.** Both call `setFocusedObject(null)` → `select(null)` → `selectPlanting(null)` → `setArmedPlant(null)`/`setArmedKind(null)` → `setLiveObject(null)` → `setLivePlanting(null)` (the editor page's `clear()` omits `setFocusedObject`, which it instead handles separately around `focus`). Extract a single `resetTransientEditorState()` helper on the store so the two callers can't drift and the public page stops reaching across the store's surface for seven setters. Verified by reading both files. - **`web/src/pages/PublicGardenPage.tsx:38` and `:52` duplicate `GardenEditorPage.tsx:55-60` and `:219-226`** — the `objects`/`plantings`/`plants`/`plantsById` memo block (functionally identical; the editor page just routes through intermediate `serverObjects`/`serverPlantings` locals) and the five-field `EditorGarden` copy from `full.data.garden` (line-for-line identical). This is the canonical "build editor view-model from FullGarden" transform; both pages should share a `useEditorViewModel(full)` hook or `toEditorGarden(full)` helper. Verified by reading both files. - **`internal/api/public.go:67` returns `gin.H{"enabled": false}`** while `getShareLink`/`createShareLink` return the typed `*PublicShareLink`. The disable response shape is now hand-built in the handler and will silently fall out of sync with `PublicShareLink` if it gains a field. The service's `DisablePublicShareLink` returns only `error`, so either have it return the resulting disabled `*PublicShareLink`, or have the handler return `&PublicShareLink{Enabled: false}`. Verified by reading the file and the service signature. - **`web/src/lib/shares.ts:78` `shareLinkSchema`'s `token: z.string().optional()`** imposes no format/length invariant, while the backend generates a 24-char URL-safe token (`newPublicToken` → `randToken(18)` → base64-rawurl) and the rest of the data layer (e.g. `shareRoleSchema` as `z.enum`) encodes its invariants in zod. A `.min(1).regex(/^[A-Za-z0-9_-]+$/)` would make the contract explicit and catch a backend regression. Verified by reading the file and `internal/service/service.go`. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found through the Performance lens. The change is performance-clean in my lane: - `assembleFull` is a faithful extraction of the existing `GardenFull` body — same three store calls, same map-lookup `DerivedCount` fill (no new N+1). The public path runs the identical query set as the authenticated path, no extra work. - `GetGardenByPublicToken` queries on the unique-indexed `public_token` column (migration 0004's `idx_gardens_public_token`), so the unauthenticated lookup is a single indexed point read — no scan, no missing pagination. - `newPublicToken` allocates 18 bytes vs. the session's 32 — trivially less, fine. - No new hot loops, no unbounded growth, no missing limits introduced. The public endpoint has no rate limiting, but that's an availability/security concern, not a per-request efficiency regression — outside my lane. </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run context deadline exceeded </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Blocking issues found</summary> **Blocking issues found** - **`internal/service/public.go:43-44`** — `PublicGarden` clears `OwnerID` on the top-level garden but **not on nested `Plants`**. `assembleFull` loads referenced plants via `ListReferencedPlants`, and each `domain.Plant` carries an `OwnerID *int64` (the user ID of the plant's creator). For custom plants this leaks the owner's user ID — exactly the kind of identity disclosure the PR says it prevents by zeroing `Garden.OwnerID`. Worse, if a co-editor created a planting with their own custom plant, that co-editor's user ID is leaked too. **Fix:** After `assembleFull`, iterate over `full.Plants` and nil out `OwnerID` so the field is omitted from JSON: ```go for i := range full.Plants { full.Plants[i].OwnerID = nil } ``` Also add assertions in `TestPublicShareLink` and `TestPublicShareLinkAPI` verifying that no `ownerId` appears in plants/objects of the public payload. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict:** No material issues found **Review notes (verified):** - **Token entropy:** `randToken(18)` → 18 bytes = 144 bits, base64url raw (no padding) → 24 characters. This matches the PR description and is a reasonable capability-token size (comparable to e.g. Dropbox / Google Docs share tokens). ✅ - **Public GET semantics:** `PublicGarden` trims and rejects empty tokens, returns `domain.ErrNotFound` for unknown/rotated/disabled tokens, which `writeServiceError` maps to 404. Never redirects. ✅ - **Token isolation from normal garden payload:** `gardenColumns` does **not** include `public_token`; `GetGardenByPublicToken` selects only `gardenColumns`, so the token cannot leak through the normal garden response. Confirmed by reading `internal/store/gardens.go:13`. ✅ - **Minimization of leaked info:** `full.Garden.MyRole = ""` and `full.Garden.OwnerID = 0` are correctly set before returning the public payload. The frontend `gardenSchema` parses `ownerId` as `z.number()`, so `0` parses fine. ✅ - **ACL on management endpoints:** All three management handlers (`getShareLink`, `createShareLink`, `deleteShareLink`) call `mustActor(c)`, which requires an authenticated session. The service layer further enforces `roleOwner` via `requireGardenRole`. ✅ - **CSRF protection:** `v1.Use(h.csrfGuard())` applies to all state-changing routes under `/api/v1`, including the new `POST`/`DELETE` share-link endpoints. Safe methods (`GET`) are explicitly exempted, which is correct for the public read endpoint. ✅ - **Route collision:** `g/$token` does not collide with `gardens/$gardenId`; the router distinguishes them correctly. ✅ - **Frontend `useEffect` cleanup:** The `PublicGardenPage` effect resets transient editor state on mount with `token` as the dependency, which correctly re-runs when the URL token changes. ✅ Nothing in the correctness lens requires a finding. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **No material issues found** I verified the PR through the code-cleanliness lens and found nothing material to report. **What I checked:** - **Backend structure**: `internal/api/public.go` and `internal/service/public.go` follow the existing thin-handler / thick-service pattern consistently. The `linkState` helper is appropriately scoped and simple. - **Refactoring quality**: Extracting `assembleFull` in `internal/service/objects.go` cleanly eliminates duplication between authenticated `GardenFull` and the new public read — this is exactly the kind of DRY improvement this lens rewards. - **Naming**: `PublicGarden`, `EnablePublicShareLink`, `PublicLinkSection`, `usePublicGarden`, etc. are all descriptive and consistent with surrounding code. - **Frontend patterns**: The new `publicGarden.ts` hook follows the identical `queryOptions` + `useQuery` pattern used by `gardens.ts` and `objects.ts`. `PublicGardenPage.tsx` is appropriately sized and reuses existing `GardenCanvas`, `toEditorObject`, and `toEditorPlanting` helpers rather than duplicating transformation logic. - **Store layer**: `GetGardenByPublicToken`, `GetGardenPublicToken`, and `SetGardenPublicToken` in `internal/store/gardens.go` match the existing store patterns (error wrapping, `RowsAffected` check, `sql.NullString` for nullable text, `var val any` for driver arguments — all idiomatic for Go 1.26). - **Router**: Adding `publicGardenRoute` to `router.tsx` follows the exact `createRoute` pattern of its siblings; the inline comment explaining the missing `beforeLoad` guard is appropriate. The change is well-structured, follows established patterns, and introduces no readability or maintainability regressions. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found I scrutinized this PR through the **performance** lens and found nothing material to report. Here is what I verified: **Backend queries** - `internal/service/public.go:28-46` — `PublicGarden` does a single indexed lookup (`GetGardenByPublicToken`) followed by `assembleFull`. The latter runs the same three queries (`ListObjectsForGarden`, `ListActivePlantingsForGarden`, `ListReferencedPlants`) that `GardenFull` already ran before this PR, so there is **no new N+1 or extra query overhead**. - `internal/store/gardens.go:162-172` — `GetGardenByPublicToken` filters by the unique partial index `idx_gardens_public_token`; the planner will use the index. No table-scan risk. - `internal/store/gardens.go:196-214` — `SetGardenPublicToken` is a single-row `UPDATE` by primary key. Correct and cheap. **Frontend** - `web/src/pages/PublicGardenPage.tsx:38-41` — The four `useMemo` hooks around `objects`, `plantings`, `plants`, and `plantsById` are reasonable and prevent redundant re-creation on stable data. - `web/src/lib/publicGarden.ts` — `staleTime: 30_000` and `retry: false` are sensible defaults for a public read endpoint. The change is a clean refactor with no efficiency regressions, unbounded growth, or hot-path allocations introduced. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`web/src/components/gardens/ShareGardenModal.tsx:167`** — Missing cleanup for the `setTimeout` in `copy()`. If `PublicLinkSection` unmounts before the 1500 ms timeout fires (e.g., user closes the Share dialog right after clicking Copy), React will warn about a state update on an unmounted component. Multiple rapid clicks on Copy also create overlapping timeouts that can prematurely clear the copied state. **Fix:** Track the timeout ID in a ref and clear it both before starting a new one and in a `useEffect` cleanup: ```tsx const timeoutRef = useRef<number | null>(null) useEffect(() => () => { if (timeoutRef.current) window.clearTimeout(timeoutRef.current) }, []) async function copy() { if (!url) return try { await navigator.clipboard.writeText(url) setCopied(true) if (timeoutRef.current) window.clearTimeout(timeoutRef.current) timeoutRef.current = window.setTimeout(() => setCopied(false), 1500) } catch { /* … */ } } ``` - **`internal/api/public.go:48–49`** — `createShareLink` intentionally swallows `ShouldBindJSON` errors so a malformed body defaults to `rotate=false`. While documented, a client sending `\"rotate\": \"yes\"` (string instead of bool) silently gets a different result than requested with no feedback. This is a design choice, not a bug, but worth noting as an unhandled edge case in the API contract. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-19 06:17:18 +00:00
Address review: redact plant owner ids, cache-control, 400, DRY reset
Build image / build-and-push (push) Successful in 15s
0842618ad1
- Security (2-model, security+correctness): the public payload zeroed
  Garden.OwnerID but referenced custom plants still carried Plant.OwnerID,
  leaking the owner's user id to anonymous viewers. Redact plant owner ids
  too, and assert it in the service test.
- Set Cache-Control: no-store on the public read so a capability-URL response
  isn't held in a shared proxy cache and always reflects the live garden.
- createShareLink now 400s on a present-but-malformed JSON body instead of
  silently enabling (an empty body still means enable-without-rotate).
- Extract the transient-editor-state reset into a shared resetTransient store
  action (3-model finding) and use it from PublicGardenPage.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit 9968c06243 into main 2026-07-19 06:18:31 +00:00
steve deleted branch feat/public-share-link 2026-07-19 06:18:31 +00:00
Sign in to join this conversation.