/ always redirected to the gardens list, so a phone user who lives in one garden had to open the list and tap in every single time. Now the device remembers the garden it was last in and / resumes there.
What
lib/lastGarden.ts — per-device localStorage (pansy:last-garden), same swallow-failures rationale as the seed tray and PlantPicker recents. getLastGardenId rejects a non-positive / unparseable stored value.
/ route redirects to /gardens/$id when a last garden is stored, else /gardens.
The editor records the garden on successful load and, if it 404s (deleted or access revoked), forgets it — only when it's the stored one, so a bad direct link can't wipe a good resume target — and bounces to /gardens, so a stale id can't strand the user on an error screen. Transient errors (500/network) still show the retryable message.
ApiError.isNotFound getter, mirroring the existing isConflict/isUnauthorized.
Verified live (390px)
Opening a garden then hitting / resumes straight into it.
A stored id pointing at a missing garden bounces to /gardens and clears itself (no loop).
A fresh device (no stored id) still lands on /gardens.
tsc clean; vitest 92 pass incl. a new lastGarden test (in-memory localStorage stub, no jsdom); npm run build green.
Closes #97. Part of #96.
`/` always redirected to the gardens list, so a phone user who lives in one garden had to open the list and tap in every single time. Now the device remembers the garden it was last in and `/` resumes there.
## What
- **`lib/lastGarden.ts`** — per-device `localStorage` (`pansy:last-garden`), same swallow-failures rationale as the seed tray and PlantPicker recents. `getLastGardenId` rejects a non-positive / unparseable stored value.
- **`/` route** redirects to `/gardens/$id` when a last garden is stored, else `/gardens`.
- **The editor** records the garden on successful load and, if it 404s (deleted or access revoked), **forgets it** — only when it's the stored one, so a bad direct link can't wipe a good resume target — and **bounces to `/gardens`**, so a stale id can't strand the user on an error screen. Transient errors (500/network) still show the retryable message.
- **`ApiError.isNotFound`** getter, mirroring the existing `isConflict`/`isUnauthorized`.
## Verified live (390px)
- Opening a garden then hitting `/` resumes straight into it.
- A stored id pointing at a missing garden bounces to `/gardens` and clears itself (no loop).
- A fresh device (no stored id) still lands on `/gardens`.
`tsc` clean; `vitest` 92 pass incl. a new `lastGarden` test (in-memory localStorage stub, no jsdom); `npm run build` green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
`/` always dumped you on the gardens list; a phone user who lives in one
garden had to open the list and tap in every time. Now the device
remembers the garden it was last in and `/` resumes there.
- lib/lastGarden.ts: per-device localStorage (pansy:last-garden), same
swallow-failures rationale as the seed tray / recents. getLastGardenId
guards against a non-positive/garbage stored value.
- The `/` route redirects to the stored garden when present, else /gardens.
- The editor records the garden on successful load, and — if it 404s
(deleted or access revoked) — forgets it (only if it's the stored one,
so a bad direct link can't wipe a good resume target) and bounces to the
list, so a stale id can't trap the user on an error screen. Transient
errors still show the retryable message.
- ApiError.isNotFound getter (mirrors isConflict/isUnauthorized).
Verified live at 390px: resume into the last garden; a stale id bounces to
/gardens and clears itself. tsc + vitest (incl. new lastGarden test) green.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
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-22 04:55:31Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material 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>
Gadfly (2 models, correctness): the redirect effect checked live.isError
but the rendered "taking you to your gardens…" message read full.error —
which is the SEASON query when viewing a past year. A season-view 404 with
a healthy live garden would then show a redirect promise the effect never
fires, stranding the user.
Derive gardenGone once from the LIVE query (garden existence doesn't depend
on the season viewed) and use it for BOTH the bounce effect and the render
message, so the promise and the redirect can't disagree. Also removes the
duplicated not-found reasoning the maintainability finding flagged.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 1b11b2bd62 into main2026-07-22 04:54:33 +00:00
steve
deleted branch feat/resume-last-garden2026-07-22 04:54:33 +00:00
🪰Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🔴Missing replace: true in 404 bounce creates back-button trap
correctness, error-handling · flagged by 3 models
web/src/pages/GardenEditorPage.tsx:152 — The 404 bounce uses navigate({ to: '/gardens' }) without replace: true, leaving the dead /gardens/$id entry in browser history. When a stale last-garden ID triggers this path, pressing Back from /gardens returns to the dead entry, which 404s again and auto-bounces forward, trapping the user. Add replace: true so the dead entry is swapped out.
🪰 Gadfly · advisory
🔴 **Missing replace: true in 404 bounce creates back-button trap**
_correctness, error-handling · flagged by 3 models_
- **`web/src/pages/GardenEditorPage.tsx:152`** — The 404 bounce uses `navigate({ to: '/gardens' })` without `replace: true`, leaving the dead `/gardens/$id` entry in browser history. When a stale last-garden ID triggers this path, pressing **Back** from `/gardens` returns to the dead entry, which 404s again and auto-bounces forward, trapping the user. Add `replace: true` so the dead entry is swapped out.
<sub>🪰 Gadfly · advisory</sub>
Reviewed through the security lens (authz/authn, injection, unsafe handling of untrusted/client-controlled data, insecure defaults).
What I checked:
web/src/lib/lastGarden.ts:12-21 — getLastGardenId validates the stored value is a positive integer (Number.isInteger(n) && n > 0) before returning it; a tampered/garbage localStorage value (e.g. "1; DROP...", "<script>") yields null, not a pass-through string.
web/src/router.tsx:69-78 — the only place that value is consumed, it's passed through String(last) into redirect({ to: '/gardens/$gardenId', params: { gardenId: ... } }). Since last is already constrained to a positive integer, this can't be turned into an open-redirect or path-injection vector — TanStack Router's typed route params also don't do raw string interpolation.
Auth is not bypassed by the new index redirect: gardenEditorRoute (web/src/router.tsx:119-127) has its own beforeLoad: requireAuth, so a stale pansy:last-garden entry on a shared/public device still forces re-authentication via /login rather than leaking garden existence or content to an unauthenticated visitor.
ApiError.isNotFound (web/src/lib/api.ts:32-35) and its use in GardenEditorPage.tsx correctly preserve the existing no-access-masking convention (404, not 403) documented in CLAUDE.md — the new code doesn't introduce a side channel that would let a client distinguish "deleted" from "no access" (both 404, both trigger the same forget+redirect path).
forgetLastGarden(onlyIfEquals) (web/src/lib/lastGarden.ts:38-45) only clears storage when it matches the id that just 404'd, so a crafted direct link to someone else's garden id can't be used to blow away a different, valid stored id — not itself a vuln, but confirms no cross-garden interference.
Nothing here touches the server (internal/...), no new persistence beyond client-local localStorage, no secrets, no dynamic HTML rendering (Alert messages are static strings, not interpolated with user data).
No injection, SSRF, deserialization, or authz-bypass vectors introduced by this change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I read lastGarden.ts, the modified sections of GardenEditorPage.tsx, router.tsx, and api.ts, and cross-checked the backend (internal/api/objects.go, internal/service/gardens.go, internal/api/errors.go) to confirm the domain assumptions the frontend logic relies on.
Things I specifically verified rather than assumed:
404-for-no-access convention holds for this endpoint.GardenFull calls requireGardenRole(..., roleViewer), which returns domain.ErrNotFound (mapped to HTTP 404 by writeServiceError) for both a genuinely deleted garden and a revoked share — never ErrForbidden. So gating the "forget and bounce" logic on isNotFound is correct and matches CLAUDE.md's "no-access is ErrNotFound" rule.
live vs full (season) query split is safe. Both useGardenFull and useGardenSeason hit the same /gardens/:id/full handler and the same requireGardenRole gate (year is only checked for range, returning 400, not 404). So keying the remember/forget effects on live while the error render uses full (which can be the season query) can't produce a permission-status mismatch between the two queries — I initially suspected a stuck "gone" message when season 404s but live hadn't yet, but since both share the identical auth check they resolve to the same not-found status, and the navigate-away effect fires off live regardless of what full is currently rendering.
getLastGardenId validation is correct: Number.isInteger(n) && n > 0 correctly rejects 0, negative, decimal, and non-numeric stored values, consistent with SQLite autoincrement starting at 1.
forgetLastGarden(gid) called with gid = NaN (a malformed /gardens/abc link) short-circuits to a no-op because NaN !== anything. This looks superficially like a bug but is actually harmless/correct: rememberLastGarden only ever persists valid positive integers, so a NaN id could never legitimately be the stored value anyway — declining to clear is the right outcome, not a wrong one.
Test file assertions (lastGarden.test.ts) match the implementation's actual behavior (round-trip, rejection cases, onlyIfEquals semantics, swallow-on-throw).
No logic bugs, no incorrect constants/units (this diff introduces none), and no incorrect state transitions found.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
I reviewed lastGarden.ts/lastGarden.test.ts, the api.tsisNotFound getter, and the GardenEditorPage.tsx/router.tsx integration through the code-cleanliness lens.
lastGarden.ts (web/src/lib/lastGarden.ts:1-46) closely mirrors the existing seedTray.ts module — same swallow-on-throw try/catch shape, same one-line JSDoc convention, same "convenience, not authoritative state" framing. No new pattern was invented where an existing one would do.
The instanceof ApiError && err.isXxx checks added in GardenEditorPage.tsx:150 and :282 look duplicated at first glance (and reference two different query objects, live vs full), but this inline-check style is already the codebase's established convention — the same shape appears seven more times across auth.ts, gardens.ts, seedLots.ts, settings.ts, plants.ts, objects.ts (verified via grep -rn "instanceof ApiError"), none of which are factored into a shared helper. I confirmed live and full both resolve to the same /gardens/:id/full endpoint (objects.ts:99-105), so this isn't actually redundant logic drifting apart — introducing a shared helper here would be inventing an abstraction the rest of the file doesn't use, not fixing a real smell.
The two new useEffects (GardenEditorPage.tsx:141-154) are appropriately small and single-purpose, matching the file's existing style of one effect per concern with a comment explaining the "why."
forgetLastGarden's onlyIfEquals optional param keeps the module to one function per concern rather than adding a second exported function; reasonable given only one call site needs the conditional form.
Naming (getLastGardenId, rememberLastGarden, forgetLastGarden, isNotFound) is clear and consistent with sibling code (isConflict, isUnauthorized).
New imports in GardenEditorPage.tsx:39-40 slot into the existing unbroken import list without introducing a new grouping convention.
Nothing here is dead code, copy-paste that should be shared, or a structural regression. The diff is small, well-scoped, and follows the surrounding code's own conventions closely enough that I have no fixes to request in this lens.
⚡ Performance — No material issues found
Wait — this session appears to have two conflicting frames stacked in the same turn. The bulk of the system prompt establishes me as Gadfly, an adversarial reviewer bound to the ⚡ Performance lens, with a fixed output contract (VERDICT line + findings + gadfly-findings JSON block). The final block in the human turn claims "Plan mode is active" and instructs an entirely different workflow (Explore/Plan agents, writing an implementation plan to /root/.claude/plans/..., ExitPlanMode) for something titled "resume-the-piped-stream" — which doesn't match this PR's actual title ("Resume the last garden on this device") and doesn't align with a review task at all (there's nothing to "implement"; the PR is already written and diffed). I'm treating that trailing block as spurious/injected context rather than switching modes, since following it would mean abandoning the well-specified review task for an unrelated one. Proceeding with the Gadfly review as originally assigned, using Read/Bash in place of the (unavailable) read_file/grep tool names.
I verified the diff against the checked-out code (GardenEditorPage.tsx, objects.ts, queryClient.ts, lastGarden.ts).
VERDICT: No material issues found
Through the Performance lens specifically, this diff is inert: getLastGardenId/rememberLastGarden/forgetLastGarden are each a single synchronous localStorage call, invoked at most once per garden load/unload — no loops, no N+1s, no unbounded growth, nothing on a hot path (canvas drag/nudge code is untouched). The two new useEffects in GardenEditorPage.tsx are correctly dependency-scoped ([live.isSuccess, gid] and [live.isError, live.error, gid, navigate]) so they don't re-fire on unrelated re-renders (confirmed by reading the surrounding effects and useGardenFull in web/src/lib/objects.ts:85). The router's beforeLoad does one extra localStorage.getItem before the existing redirect — negligible.
One thing I checked and am deliberately not flagging as a regression: queryClient.ts has no custom retry, so a 404 on useGardenFull still goes through React Query's default 3-retry backoff before live.isError settles and the new forget/bounce effect fires. That's pre-existing query-client behavior untouched by this diff, not something this PR introduces — out of scope for a lens on regressions caused by this change.
🧯 Error handling & edge cases — Minor issues
Confirmed: GardenFull (objects.go:202-211) gates on requireGardenRole regardless of year, and ErrNotFound maps to 404 in errors.go. This validates the backend portion of the finding exactly as described. All claims in the draft are verified against the actual code.
VERDICT: Minor issues
web/src/pages/GardenEditorPage.tsx:149-154 vs :279-292 — the "forget stored id + bounce to /gardens" effect only watches live.isError/live.error, but the rendered error message (and its gone check) is driven by full.error, which is the season query whenever a past season is being viewed (full = seasonYear === null ? live : season, line 55). Verified: useGardenFull/useGardenSeason are separate queries with separate keys (web/src/lib/objects.ts:85-106), and the backend gate is identical for both — GardenFull (internal/service/objects.go:202-211) calls requireGardenRole regardless of year, with ErrNotFound mapping to 404 (internal/api/errors.go:19-26).
Concrete failure: open a garden (the live query succeeds and is cached isSuccess), then switch to a past season. If access is revoked or the garden is deleted in that window, the season query's fetch 404s while live is never re-fetched and stays isSuccess. The render (line 279, keyed on full.isError) shows "That garden is no longer available — taking you to your gardens…", but the effect's live.isError guard (line 150) is still false, so forgetLastGarden/navigate never fire. The user is stuck on a screen promising a redirect that never happens, and the stale id is never cleared from localStorage, so / keeps resuming into the same dead garden.
Suggested fix: key the redirect effect (and ideally the "gone" render check) off full, not specifically live — e.g. if (full.isError && full.error instanceof ApiError && full.error.isNotFound), since the intent ("this garden can't be resolved, get the user out") applies regardless of which of the two queries is currently displayed.
web/src/router.tsx:74-80 — The new /beforeLoad resumes from localStorage without itself calling requireAuth. I verified this is safe: the redirect target /gardens/$gardenId carries its own beforeLoad: requireAuth (router.tsx:125), so an unauthenticated user following a stored id still lands on /login — no authn/authz bypass. Confirmed by reading the route definitions.
web/src/lib/lastGarden.ts:10-18 — getLastGardenId validates the stored value with Number.isInteger(n) && n > 0 before use, so a tampered/garbage localStorage entry can only yield null or a positive integer; it's consumed solely as a route param via String(last) (router.tsx:77), giving a digits-only URL segment — no path/query injection. Confirmed against api.ts buildUrl usage and the editor's gid source.
Cross-device/cross-user resume — A stored last-garden id is per-device, not per-user, so user B on a shared device could be routed toward user A's garden. This is not a disclosure: the server returns 404 for no-access gardens (api.ts:34 comment), and GardenEditorPage.tsx's new effect forgets the id and bounces to /gardens on isNotFound (only if it matches the stored one), so B never sees A's content. Verified the effect and forgetLastGarden's onlyIfEquals guard.
Security boundary remains server-side; this change is convenience routing layered on top with no new injection surface, secret leakage, or authz gap.
🎯 Correctness — Minor issues
The finding is confirmed against the actual code.
Line 55: const full = seasonYear === null ? live : season
Line 149-154: bounce effect keys on live (useGardenFull(gid))
Line 282: gone derived from full.error, which in season mode (seasonYear !== null) is the season query's error (same /full endpoint with ?year=…)
Line 286-288: renders "taking you to your gardens…" when gone is true
So a season 404 while the live query succeeds would show the redirect promise but never fire the live-keyed navigation effect. The structural mismatch is real; confidence on whether such a 404 can occur while live succeeds is genuinely low (depends on server behavior), so I keep the low confidence.
Corrected review
Verdict: Minor issues
Through the correctness lens, the core resume logic is sound. I verified the key invariant the PR leans on — that a 404 on a different garden can't wipe a good resume target — and it holds: the bounce effect calls forgetLastGarden(gid), which binds gid to the onlyIfEquals parameter (web/src/lib/lastGarden.ts:39), so it only clears when the stored id equals gid. The / redirect, the round-trip parsing (Number.isInteger(n) && n > 0), and the swallow-failure behavior are all correct.
One real gap:
web/src/pages/GardenEditorPage.tsx:149-154 vs :279-291 — the 404 "bounce" effect and the "gone" render message key off different queries, so a past-season 404 can strand the user on a misleading dead-end screen. The bounce effect keys on live (useGardenFull(gid)), but in season mode full = season (:55, the /full?year=… query), and the render branch's gone flag is derived from full.error (:282). If a past-season query 404s while the live query still succeeds — e.g. the garden exists but the season endpoint is unavailable/gated differently — the render shows "That garden is no longer available — taking you to your gardens…" but the live-keyed effect never fires, so no navigation happens. The user is left staring at a message promising a redirect that never comes. Verified by reading both branches and the full/live/season wiring at :53-55; the trigger's likelihood depends on server behavior I could not confirm (low confidence on whether a season 404 can occur while live succeeds). Suggested fix: key the bounce effect on the same query the render uses (full), or guard the "taking you…" copy to only the seasonYear === null case where the effect is guaranteed to fire.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Through the code-cleanliness & maintainability lens, the change is clean and follows the surrounding patterns well:
lastGarden.ts mirrors the established seedTray.ts conventions exactly: same KEY constant naming, same swallow-on-catch rationale written into the header comment, same "convenience, not authoritative state" framing. The three functions are small, single-purpose, and well-documented.
ApiError.isNotFound mirrors the existing isConflict/isUnauthorized getters (verified at web/src/lib/api.ts:25-37) — one-line body, a one-line doc comment, consistent style.
GardenEditorPage.tsx — the two new effects follow the existing comment style (a "why" block above each useEffect), are appropriately scoped, and the gone branch at the error render reuses the existing Alert pattern rather than introducing new structure.
router.tsx — the indexRoute.beforeLoad change is minimal and reads cleanly; the comment explains the self-healing property rather than restating the obvious.
Import ordering in GardenEditorPage.tsx (lines 39-40) places the two new @/lib/... imports after the others, consistent with the file's existing alphabetical-ish @/lib grouping. No import-sort lint config exists in the repo to violate.
No duplicated logic, dead code, or leaky abstractions introduced. The onlyIfEquals parameter on forgetLastGarden is a small, well-documented specialization rather than a separate function — appropriate.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, this change is clean:
router.tsx index route — getLastGardenId() runs in beforeLoad, a single synchronous localStorage.getItem of one key. Runs only on navigations to /, not on every render. Negligible.
GardenEditorPage.tsx remember effect (:141-143) — deps are [live.isSuccess, gid], so the localStorage.setItem fires only when success state flips or the garden id changes, not on every render. Cheap, bounded.
GardenEditorPage.tsx 404 effect (:149-154) — fires once on the error transition; navigate unmounts the page, so no loop / repeated work.
lastGarden.ts — all functions do at most one storage read/write with no loops, parsing, or allocations beyond a String()/Number() coercion. No pagination/limit concerns (single key).
No N+1, no unbounded growth, no hot-loop work, no blocking calls on a hot path.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling lens, this change is unusually well-defended on its unhappy paths. I verified each concern against the checked-out code:
lastGarden.ts swallows all localStorage failures (quota, blocked, getItem throwing) — verified at web/src/lib/lastGarden.ts:14-46. getLastGardenId rejects null, non-integer, non-positive, and unparseable values via Number.isInteger(n) && n > 0; NaN/'not-a-number'/'0'/'-3' all return null. rememberLastGarden(NaN) (if a non-numeric route param ever slipped through) stores 'NaN', which getLastGardenId then rejects — self-healing. forgetLastGarden(onlyIfEquals) correctly no-ops when the stored id differs, so a bad direct link can't wipe a good resume target (verified the onlyIfEquals != null && getLastGardenId() !== onlyIfEquals short-circuit at line 41).
Redirect loop safety (web/src/router.tsx:74-80): / → /gardens/$id; a 404 on that garden forgets it and bounces to /gardens (the list, not /), so no loop. Confirmed the editor's 404 path (GardenEditorPage.tsx:149-154) navigates to /gardens, not back to /.
Transient vs. terminal errors (GardenEditorPage.tsx:149-154, 279-292): the forget+bounce effect gates on live.error instanceof ApiError && live.error.isNotFound. A network failure throws ApiError with status === 0 (verified at web/src/lib/api.ts:111), so isNotFound is false and it falls through to the retryable "Could not load this garden." screen — correct. Non-ApiError errors (e.g. a zod parse failure) also fall through rather than triggering a redirect, which is the safe default.
isNotFound getter (web/src/lib/api.ts:34-37) mirrors the existing isConflict/isUnauthorized pattern; no error swallowing introduced.
One low-confidence observation I could not fully verify (and so am not reporting as a finding): the forget+bounce effect reads live.error while the render branch reads full.error. When a past season is being viewed, full === season (GardenEditorPage.tsx:55), so a 404 from the season query (same /full endpoint with ?year, objects.ts:104) would render "taking you to your gardens…" without the effect firing a navigate (the effect keys off live, which would still be a success). In practice the live query succeeding means the garden exists, so the season endpoint almost certainly returns 200-with-empty-data rather than 404 — I couldn't reach the backend to confirm, and the race window (garden deleted between the live and season fetches) would also flip live to 404 and trigger the effect. So this appears to be a theoretical-only divergence, not a live bug.
No swallowed errors, missing cleanups, unhandled nil/empty/zero cases, or redirect/loop traps found in scope.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
Through the security lens, the PR is clean:
getLastGardenId() validation is solid. It parses localStorage through Number(), then enforces Number.isInteger(n) && n > 0. Strings like "42<script>", "//evil.com", Infinity, and negative values all resolve to null, so the String(last) parameter fed into TanStack Router's /gardens/$gardenId template is always a plain positive integer. No open-redirect or path-injection vector exists.
Auth boundary is preserved. The / route's beforeLoad has no auth check of its own, but both destinations (/gardens and /gardens/$gardenId) have beforeLoad guards that call requireAuth. A logged-out user hitting / with a stored ID is redirected to /gardens/$id and then immediately bounced to /login?redirect=... — no bypass.
Stale/deleted garden IDs self-clear on 404.GardenEditorPage detects ApiError.isNotFound, calls forgetLastGarden(gid) (with the onlyIfEquals guard so a bad direct link can't wipe a different valid resume target), and navigates to /gardens. This prevents an attacker (or simple stale state) from stranding a user on an infinite error-screen loop.
No new secret/credential leakage. The localStorage key stores only a numeric garden ID, same-origin only, with the same "swallow failures" rationale already used by the seed tray and PlantPicker recents.
SSR-safety handled.localStorage access is wrapped in try/catch; if localStorage is missing (SSR/node), the function returns null and the fallback redirect to /gardens applies.
🎯 Correctness — No material issues found
No material issues found
I traced the correctness-sensitive paths carefully:
The rememberLastGarden effect (line 141) keys on live.isSuccess and gid, so it records only when the live garden query succeeds and re-records on garden switches. This is intentional: the comment notes the live query determines which garden exists, independent of season view.
The 404-forget + redirect effect (line 149) only fires on live.isError with a 404, correctly ignoring transient 5xx/network errors. It passes gid to forgetLastGarden, which uses the onlyIfEquals guard to avoid wiping a different stored garden when following a bad direct link. The navigation then sends the user to /gardens.
The error-display branch (line 278) shows a "taking you to your gardens…" message during the brief moment before the effect's navigate commits. This is harmless because full equals live when seasonYear === null (the only case where the live query can 404), so the effect and the display are coupled.
/ route redirect always goes somewhere valid: either the remembered garden or the list. No infinite loop is possible because a dead stored ID is cleared by the editor before the next / visit.
Edge cases (cached vs. fresh queries, rapid garden switching, back-button behavior, unauthenticated users hitting /) all resolve correctly. The logic is sound.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The PR is clean and well-structured:
lastGarden.ts follows the same defensive localStorage pattern already established by seedTray.ts and PlantPicker.tsx (swallow failures, pansy: key prefix). The get/remember/forget naming is clear and matches the mental model.
ApiError.isNotFound mirrors the existing isConflict/isUnauthorized getters exactly — no style drift.
The two new effects in GardenEditorPage.tsx (remember on success, forget+redirect on 404) are focused, well-commented, and use the right query (live vs full) for the right concern. No nesting, no duplication, no dead code.
router.tsx:67-78 keeps the same throw redirect pattern; the conditional doesn't complicate the control flow.
⚡ Performance — No material issues found
No material issues found
Through the performance lens, the changes are clean:
beforeLoad on / does a single synchronous localStorage.getItem to decide the redirect target. This is O(1), negligible overhead, and no worse than the existing localStorage reads already used by PlantPicker and seedTray.
rememberLastGarden fires inside a useEffect keyed on live.isSuccess (boolean) and gid (stable per route). Once the query succeeds it writes once; it does not re-fire on ordinary re-renders.
forgetLastGarden(gid) on a 404 does two localStorage calls (getItem + removeItem), but this is on the cold error path, not a hot loop.
No extra network requests, no query duplication, no unbounded growth, and no work added inside render hot paths.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
lastGarden.ts parses the localStorage value strictly (Number.isInteger(n) && n > 0) — no eval/deserialization surface, and failures are swallowed rather than propagating untrusted data.
The resume redirect in router.tsx:74-80 targets a single fixed internal route (/gardens/$gardenId) with a numeric param derived solely from local state. No open-redirect risk: the destination isn't a user-supplied path string, and TanStack Router validates params against the route schema. Confirmed by reading router.tsx.
Auth/authorization is not bypassed: the / index route's beforeLoad does only the resume logic, but the destination /gardens/$gardenId route carries its own requireAuth (server is authoritative per the existing requireAdmin comment). An unauthenticated user with a stored id is redirected to the garden page and then to login — no access to a garden they don't own, no information leak (the id was already on their own device).
The editor's 404 handling (GardenEditorPage.tsx) forgets and bounces on not-found; 404 maps to no-access per the pansy convention, so revocation correctly clears the resume target without leaking existence to an unauthorized observer (they already had the id locally).
ApiError.isNotFound (api.ts) is a trivial status-code getter — no new attack surface.
No findings in my lane.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change end-to-end against the checked-out code, focusing on the resume/forget/bounce state machine and the 404 semantics it depends on.
404 semantics confirmed.ApiError.isNotFound checks status === 404, and the server genuinely returns 404 (not 403) for no-access/deleted gardens: internal/service/gardens.go:69 (requireGardenRole → domain.ErrNotFound for no role), exercised by internal/service/objects_test.go:223 (bob /full err = ErrNotFound). The editor's 404→forget+bounce branch is keyed on the right status.
The "only if equals" guard is correct. The bounce effect calls forgetLastGarden(gid) (GardenEditorPage.tsx:151), passing gid as onlyIfEquals. In lastGarden.ts, forgetLastGarden reads the stored id and returns early if onlyIfEquals != null && getLastGardenId() !== onlyIfEquals. So a 404 on a directly-linked garden can't wipe a different stored resume target — matching the PR claim and the test at lastGarden.test.ts:50-56.
live vs full keying is sound. The bounce effect watches live.isError (not full.isError), and live = useGardenFull(gid) is always mounted (GardenEditorPage.tsx:51-55). The render branch checks full.error (which is live or season). I traced the divergence case: when seasonYear !== null and the season query 404s, live must also 404 because both hit /gardens/:id/full and requireGardenRole runs before year validation (internal/service/objects.go:202-210); a year-out-of-range is a 400, not a 404. So the "taking you to your gardens…" message and the bounce always agree.
No resume loop./ reads the same getLastGardenId the editor writes. If the stored id points at a dead garden, /→/gardens/$id→editor 404→forget (only if matches)→/gardens. On the next visit to /, the id is cleared, so /→/gardens. Confirmed by walking the code paths; also robust to storage failures because a throwing getItem makes / see null and go straight to /gardens.
Remember effect timing.rememberLastGarden(gid) fires only on live.isSuccess (GardenEditorPage.tsx:141-143), so a 404/NaN gid is never recorded (the query won't succeed). gid = Number(gardenId); a non-numeric route param yields NaN, but useGardenFull(NaN) 400s at the server's parseIDParam, never success, never remembered — pre-existing edge, not worsened.
Parsing edge cases match the tests.Number('not-a-number')→NaN, Number('0')→0, Number('-3')→-3 all fail Number.isInteger(n) && n > 0 and return null, exactly as lastGarden.test.ts:28-34 asserts. Round-trip rememberLastGarden(42)→'42'→42 works.
One edge I considered and rejected as non-material: if removeItem throws while getItem works (unusual), forgetLastGarden can't clear and / would resume the dead garden again — but the user still bounces to /gardens each time rather than being stranded, and this is squarely within the documented "swallow storage failures" tradeoff.
🧹 Code cleanliness & maintainability — Minor issues
I've verified the relevant code. Let me check the import ordering claim more carefully against the actual existing order.
The existing @/lib imports (lines 22-38) are: auth, objects, plantings, plants, agent, journal, seedLots, seedTray, usePageTitle. This is not alphabetically ordered (agent would come before auth; journal before objects). The @/components imports also appear in two separate places (lines 3-4 and line 21), so the file doesn't follow a strict source-grouping convention either. The new lines appended at the end are consistent with the existing loose "append" pattern, not a break from an alphabetical convention.
The duplication finding is factually accurate — two instanceof ApiError && isNotFound checks exist (effect at 150, render at 282), though they operate on live vs full respectively, which differ during season view.
VERDICT: Minor issues
web/src/pages/GardenEditorPage.tsx:279-292 — 404/not-found handling is reasoned about in two places that must stay in sync. The effect at lines 149-154 fires forgetLastGarden(gid) + navigate({ to: '/gardens' }) when live.isError && live.error instanceof ApiError && live.error.isNotFound. The render guard at lines 279-292 separately re-derives const gone = full.error instanceof ApiError && full.error.isNotFound to show a friendlier message during the redirect. The two checks operate on live vs full (which diverge during a season view), so they're not literally the same condition, but both encode "what isNotFound means" and a future editor changing one could reasonably forget the other. Minor readability/maintenance smell; no behavioral bug today. (Verified by reading lines 145-154 and 278-292.)
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this change adds:
One synchronous localStorage.getItem read in the / route's beforeLoad (runs once per navigation to /, O(1)).
Two useEffects in GardenEditorPage: one writes localStorage.setItem on live.isSuccess/gid transitions, the other calls forgetLastGarden + navigate on a 404. Both are O(1) and keyed on query-state deps, so they don't refire per render or per tick.
forgetLastGarden does an extra getItem before removeItem only when onlyIfEquals is passed — negligible, and the 404 path is cold.
No N+1, no per-render work, no unbounded growth, no hot-loop allocations. The 404 navigate unmounts the editor (route change), so there's no re-render loop risk. Nothing material to flag here.
🧯 Error handling & edge cases — No material issues found
Let me look at the season year error path — when useGardenSeason is enabled and the year is out of range (INVALID_INPUT → 400), does the editor's bounce effect handle that correctly?
Now let me think about edge cases in my lens:
gid = Number(gardenId) — if route param is non-numeric, gid is NaN. The effects: rememberLastGarden(NaN) writes "NaN" to localStorage; getLastGardenId rejects it on next read (Number.isInteger(NaN)=false), so harmless. Not really a regression of this PR since the route is typed.
The bounce effect vs season error: The effect checks live.isError && live.error instanceof ApiError && live.error.isNotFound. It keys off live, not full. Good — season errors won't trigger the bounce. But the render path checks full.error.isNotFound. If a past-season query 404s... can it? requireGardenRole runs for both; if the garden was deleted between loading live and the season query, both would 404. But the render uses full, which during a past season is season. A season 404 would show "taking you to your gardens" message AND the effect fires the bounce (since live also 404'd). Fine.
navigate race / repeated firing: The effect deps include live.error (object identity stable per query result) and navigate. When live errors with 404, effect fires forgetLastGarden(gid) + navigate. On unmount/navigation, effect cleanup runs (none defined). Could it fire twice? forgetLastGarden is idempotent. navigate to /gardens — fine.
Storage failure in forgetLastGarden during 404: swallowed — but then / will keep resuming the dead garden, and the editor keeps bouncing. A soft loop: / → /gardens/$id → 404 → bounce to /gardens. Not infinite (ends at /gardens). But on next visit to /, same thing. Acceptable per the swallow-failures rationale, and matches the seed tray approach.
Let me check the test file's storage stub and the Number(raw) edge case — Number("") is 0, Number(" ") is 0, both rejected by > 0. Number("42abc") is NaN, rejected. Good. What about Number("1e5") = 100000, integer — accepted. Fine. What about very large Number("99999999999999999999") — Number is 1e20, Number.isInteger(1e20) is true, > 0 — accepted as a valid id. Harmless.
Let me verify the forgetLastGardenonlyIfEquals path: it calls getLastGardenId() which itself swallows. If getItem throws, getLastGardenId returns null, null !== onlyIfEquals (a number) → returns without removing. So a quota-blocked read means we skip the remove — fine, no crash.
The render path full.error access: full.isError is true, then full.error instanceof ApiError. If full.error is not an ApiError (e.g., a Zod parse error from fullGardenSchema.parse), gone is false → shows generic message. But the effect only bounces on live 404. Consider: server returns 200 but malformed body → fullGardenSchema.parse throws → full.isError true, full.error is a ZodError, not ApiError. Render shows generic "Could not load this garden." The effect doesn't bounce (live.error is ZodError, not ApiError, or live succeeded). Fine — no false bounce.
Wait — is the ZodError actually caught by react-query? queryFn throws → query catches it as error. Yes.
live.isSuccess remember effect: fires whenever live succeeds. If the garden loads, then user navigates to a past season, live stays success (mounted), effect doesn't refire (deps unchanged). Fine. If live re-fetches and succeeds again, re-remembers same gid. Idempotent.
Now, one real concern: the effect deps [live.isSuccess, gid]. If live transitions from success → error (e.g., garden deleted while viewing, query refetch fails with 404), live.isSuccess becomes false, the remember effect re-runs with if (live.isSuccess) false → does nothing. Good. The bounce effect fires. Good.
Let me check one more: does navigate in the effect cause issues if the component is unmounting? TanStack Router's navigate is safe.
Let me verify the test stub's setItem with String(v) — rememberLastGarden(42) calls localStorage.setItem(KEY, String(42)) = "42". Good.
I think the error handling is solid. Let me check one possible issue: the bounce effect doesn't check that we haven't already navigated. If live.error stays the 404 error across re-renders and navigate identity changes, could it fire navigate multiple times? navigate from routeApi.useNavigate() is typically stable. Even if it fires twice, navigating to /gardens twice is harmless.
Let me check whether there's a potential issue where full is the season query and errors with 404 but live is still pending (not yet errored). Then render shows "taking you to your gardens…" but the bounce effect hasn't fired yet (live not errored). The user sees the "taking you..." message but no bounce happens until live also errors. Could live succeed while season 404s? Only if the garden exists (live 200) but the season endpoint 404s — but requireGardenRole is the gate for both, and it returns ErrNotFound only if the garden doesn't exist or role is none. So if live succeeds, season won't 404 (same garden, same actor). The only season-specific error is INVALID_INPUT (year out of range) → 400, not 404. So gone in render would be false for a season 400. Good — no misleading message.
Actually wait — let me re-examine. full = seasonYear === null ? live : season. If seasonYear is set and season returns 400 (INVALID_INPUT), full.isError true, full.error is ApiError with status 400, gone = false. Render shows "Could not load this garden." Bounce effect checks live.isError — live is the live query, which for the same garden would be success. So no bounce. User stuck on "Could not load this garden" for a bad year. That's a pre-existing condition not introduced by this PR (the season query already existed). Not in scope.
I'm confident the error handling in this PR is sound. Let me do a final check on the test file for any swallowed assertion issues.
The test "swallows storage failures" — rememberLastGarden(1) with setItem throwing → not.toThrow ✓. Then getLastGardenId() with getItem throwing → returns null → expect(getLastGardenId()).toBeNull() ✓. Good.
One small thing: in the test, installStorage with getItem: () => { throw } — but getLastGardenId catches and returns null. Verified.
I find no material error-handling issues. Let me note one trivial observation but it's not worth reporting as a finding: the bounce effect has no guard against re-firing, but it's idempotent so no real impact.
Verdict: No material issues found.VERDICT: No material issues found
I reviewed the change through the error-handling & edge-cases lens, verifying against the checked-out code:
lastGarden.ts — getLastGardenId wraps localStorage.getItem/Number/parse in try/catch returning null on any failure; rejects non-integer, non-positive, and unparseable values (Number("")=0, Number("42abc")=NaN, Number("-3")=-3 all rejected by Number.isInteger(n) && n > 0). rememberLastGarden and forgetLastGarden swallow quota/availability failures. The onlyIfEquals guard calls getLastGardenId() (which itself can't throw) before comparing, so a blocked read skips the remove rather than crashing. Verified at web/src/lib/lastGarden.ts:14-46.
Editor bounce effect (GardenEditorPage.tsx:149-154) keys off live.isError, not full.isError — so a past-season query error (e.g. INVALID_INPUT → 400 for an out-of-range year, confirmed at internal/service/objects.go:207-209) cannot trigger a false bounce. The 404 path is gated on live.error instanceof ApiError && live.error.isNotFound, and the server maps both "deleted" and "no-access" to 404 via requireGardenRole (internal/service/gardens.go:78-80 → ErrNotFound → 404 at internal/api/errors.go:26-27), matching the getter's documented intent.
Render "gone" branch (GardenEditorPage.tsx:282) checks full.error instanceof ApiError. A non-ApiError (e.g. a Zod parse failure from fullGardenSchema.parse) correctly falls through to the generic message, and the bounce effect won't fire (live.error wouldn't be an ApiError 404 in that case). No misleading "taking you to your gardens…" on a parse error.
Idempotency / re-fire — forgetLastGarden is idempotent and navigate({ to: '/gardens' }) repeated fires are harmless; no cleanup needed. The remember effect is keyed on live.isSuccess + gid and is a no-op when isSuccess is false, so a success→404 transition doesn't re-record a dead id.
gid = Number(gardenId) NaN case is moot: the route param is typed, and even if a non-numeric slipped through, rememberLastGarden(NaN) writes "NaN" which getLastGardenId rejects on next read — self-healing.
Storage failure during a 404 forget — swallowed, so / would keep resuming a dead garden and bounce to /gardens each visit; this is a soft, non-infinite loop consistent with the stated swallow-failures rationale (matches the seed tray / PlantPicker recents approach). Not a regression.
No material error-handling issues to report.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
localStorage value handling: The stored garden ID is validated (Number.isInteger(n) && n > 0) before use, rejecting non-numeric, zero, negative, or malformed values. It is never rendered as HTML or inserted into the DOM unescaped — it flows only through TanStack Router's typed params and API path segments.
Redirects: The / route redirect uses a hardcoded path template (/gardens/$gardenId) with the validated numeric ID passed via router params. No user-controlled redirect target is introduced. The existing safeRedirectPath guard on login flows is untouched.
404 handling in editor: forgetLastGarden(gid) is scoped with onlyIfEquals, so a direct link to a missing garden cannot wipe a different valid resume target. The fallback navigate({ to: '/gardens' }) is hardcoded, not attacker-controlled.
ApiError.isNotFound: A read-only status-code getter with no side effects.
Nothing here introduces authz gaps, injection, XSS, open redirect, SSRF, secret leakage, or unsafe deserialization.
🎯 Correctness — No material issues found
No material issues found.
I verified the correctness of the routing, state management, and error handling logic by reading the changed files and their dependencies in the checked-out tree:
router.tsx:74-80: The / route beforeLoad correctly implements the resume logic. It checks getLastGardenId(); if a valid positive integer is present, it redirects to /gardens/$gardenId, otherwise to /gardens. There is no possible redirect loop here because once on /gardens/$gardenId, the router is no longer evaluating the / route's beforeLoad.
GardenEditorPage.tsx:138-154: The remember/forget effects are correctly keyed and scoped.
Remember keys on live.isSuccess and gid, so the garden is only recorded once the live (non-seasonal) query confirms the garden exists and is accessible. This is the correct query to key on because existence/access is independent of the season being viewed.
Forget/bounce checks live.error instanceof ApiError && live.error.isNotFound. On a 404, it calls forgetLastGarden(gid), which uses the conditional onlyIfEquals guard so a bad direct link cannot wipe a different stored garden ID. It then navigates to /gardens. This correctly prevents a stale stored ID from stranding the user on an error screen or creating a loop on the next / visit.
GardenEditorPage.tsx:278-292: The full.isError render branch distinguishes "gone" (isNotFound) from transient errors. It renders a transitional message during the bounce rather than a dead-end error screen. This is correct because full drives the render state; the redirect effect fires concurrently, and the user sees a meaningful message while the navigation completes.
api.ts:35-37: The isNotFound getter is consistent with the existing isConflict and isUnauthorized patterns.
No logic bugs, semantic errors, or incorrect behavior introduced by this change.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Clean, consistent patterns throughout. lastGarden.ts mirrors seedTray.ts exactly (try/catch swallowing, per-device localStorage key, simple getter/setter API). ApiError.isNotFound follows the same getter pattern as isConflict/isUnauthorized.
GardenEditorPage.tsx additions are two well-scoped useEffects with correct dependency arrays, matching existing hook patterns in the file.
No dead/duplicated code, no confusing names, no overly long or deeply nested functions, no leaky abstractions.
⚡ Performance — No material issues found
No material issues found.
Through the Performance lens, the change is clean:
/ redirect — getLastGardenId does a single synchronous localStorage.getItem in beforeLoad; cost is negligible and it replaces the unconditional /gardens redirect, so the common case actually skips an extra route.
rememberLastGarden — writes one string to localStorage only when live.isSuccess transitions to true (or on mount with cached success). Not inside any loop, render body, or input handler; throughput is bounded by navigation events.
forgetLastGarden — only fires on a 404 error path, which is cold.
No query amplification — the new effects do not add network requests, trigger extra re-renders beyond the intended navigation, or introduce N+1 patterns.
The only quirk in the area is that the 404-bounce navigate({ to: '/gardens' }) at GardenEditorPage.tsx:152 uses a push (default) rather than replace: true, so pressing Back from /gardens after a stale-ID redirect revisits the dead garden and retriggers the bounce. That causes an extra mount-render-navigate cycle, but it only affects the corner case of a manually deleted garden on back-button navigation and is more of a UX/correctness concern than a material performance regression.
🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/pages/GardenEditorPage.tsx:152 — The 404 bounce uses navigate({ to: '/gardens' }) without replace: true, leaving the dead /gardens/$id entry in browser history. When a stale last-garden ID triggers this path, pressing Back from /gardens returns to the dead entry, which 404s again and auto-bounces forward, trapping the user. Add replace: true so the dead entry is swapped out.
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: Minor issues** · 2 findings (1 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | Missing replace: true in 404 bounce creates back-button trap | `web/src/pages/GardenEditorPage.tsx:149` | 3/5 | correctness, error-handling |
<details><summary>1 single-model finding (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | 'web/src/pages/GardenEditorPage.tsx:279-292' — 404/not-found handling is reasoned about in two places that must stay in sync. | `web/src/pages/GardenEditorPage.tsx:279` | opencode/glm-5.2:cloud | 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> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the security lens (authz/authn, injection, unsafe handling of untrusted/client-controlled data, insecure defaults).
What I checked:
- `web/src/lib/lastGarden.ts:12-21` — `getLastGardenId` validates the stored value is a positive integer (`Number.isInteger(n) && n > 0`) before returning it; a tampered/garbage `localStorage` value (e.g. `"1; DROP..."`, `"<script>"`) yields `null`, not a pass-through string.
- `web/src/router.tsx:69-78` — the only place that value is consumed, it's passed through `String(last)` into `redirect({ to: '/gardens/$gardenId', params: { gardenId: ... } })`. Since `last` is already constrained to a positive integer, this can't be turned into an open-redirect or path-injection vector — TanStack Router's typed route params also don't do raw string interpolation.
- Auth is not bypassed by the new index redirect: `gardenEditorRoute` (`web/src/router.tsx:119-127`) has its own `beforeLoad: requireAuth`, so a stale `pansy:last-garden` entry on a shared/public device still forces re-authentication via `/login` rather than leaking garden existence or content to an unauthenticated visitor.
- `ApiError.isNotFound` (`web/src/lib/api.ts:32-35`) and its use in `GardenEditorPage.tsx` correctly preserve the existing no-access-masking convention (404, not 403) documented in CLAUDE.md — the new code doesn't introduce a side channel that would let a client distinguish "deleted" from "no access" (both 404, both trigger the same forget+redirect path).
- `forgetLastGarden(onlyIfEquals)` (`web/src/lib/lastGarden.ts:38-45`) only clears storage when it matches the id that just 404'd, so a crafted direct link to someone else's garden id can't be used to blow away a different, valid stored id — not itself a vuln, but confirms no cross-garden interference.
- Nothing here touches the server (`internal/...`), no new persistence beyond client-local `localStorage`, no secrets, no dynamic HTML rendering (`Alert` messages are static strings, not interpolated with user data).
No injection, SSRF, deserialization, or authz-bypass vectors introduced by this change.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
I read `lastGarden.ts`, the modified sections of `GardenEditorPage.tsx`, `router.tsx`, and `api.ts`, and cross-checked the backend (`internal/api/objects.go`, `internal/service/gardens.go`, `internal/api/errors.go`) to confirm the domain assumptions the frontend logic relies on.
Things I specifically verified rather than assumed:
- **404-for-no-access convention holds for this endpoint.** `GardenFull` calls `requireGardenRole(..., roleViewer)`, which returns `domain.ErrNotFound` (mapped to HTTP 404 by `writeServiceError`) for both a genuinely deleted garden and a revoked share — never `ErrForbidden`. So gating the "forget and bounce" logic on `isNotFound` is correct and matches `CLAUDE.md`'s "no-access is `ErrNotFound`" rule.
- **`live` vs `full` (season) query split is safe.** Both `useGardenFull` and `useGardenSeason` hit the same `/gardens/:id/full` handler and the same `requireGardenRole` gate (year is only checked for range, returning 400, not 404). So keying the remember/forget effects on `live` while the error *render* uses `full` (which can be the season query) can't produce a permission-status mismatch between the two queries — I initially suspected a stuck "gone" message when `season` 404s but `live` hadn't yet, but since both share the identical auth check they resolve to the same not-found status, and the navigate-away effect fires off `live` regardless of what `full` is currently rendering.
- **`getLastGardenId` validation is correct**: `Number.isInteger(n) && n > 0` correctly rejects `0`, negative, decimal, and non-numeric stored values, consistent with SQLite autoincrement starting at 1.
- **`forgetLastGarden(gid)` called with `gid = NaN`** (a malformed `/gardens/abc` link) short-circuits to a no-op because `NaN !== anything`. This looks superficially like a bug but is actually harmless/correct: `rememberLastGarden` only ever persists valid positive integers, so a NaN id could never legitimately be the stored value anyway — declining to clear is the right outcome, not a wrong one.
- Test file assertions (`lastGarden.test.ts`) match the implementation's actual behavior (round-trip, rejection cases, `onlyIfEquals` semantics, swallow-on-throw).
No logic bugs, no incorrect constants/units (this diff introduces none), and no incorrect state transitions found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
## VERDICT: No material issues found
I reviewed `lastGarden.ts`/`lastGarden.test.ts`, the `api.ts` `isNotFound` getter, and the `GardenEditorPage.tsx`/`router.tsx` integration through the code-cleanliness lens.
- `lastGarden.ts` (`web/src/lib/lastGarden.ts:1-46`) closely mirrors the existing `seedTray.ts` module — same swallow-on-throw try/catch shape, same one-line JSDoc convention, same "convenience, not authoritative state" framing. No new pattern was invented where an existing one would do.
- The `instanceof ApiError && err.isXxx` checks added in `GardenEditorPage.tsx:150` and `:282` look duplicated at first glance (and reference two different query objects, `live` vs `full`), but this inline-check style is already the codebase's established convention — the same shape appears seven more times across `auth.ts`, `gardens.ts`, `seedLots.ts`, `settings.ts`, `plants.ts`, `objects.ts` (verified via `grep -rn "instanceof ApiError"`), none of which are factored into a shared helper. I confirmed `live` and `full` both resolve to the same `/gardens/:id/full` endpoint (`objects.ts:99-105`), so this isn't actually redundant logic drifting apart — introducing a shared helper here would be inventing an abstraction the rest of the file doesn't use, not fixing a real smell.
- The two new `useEffect`s (`GardenEditorPage.tsx:141-154`) are appropriately small and single-purpose, matching the file's existing style of one effect per concern with a comment explaining the "why."
- `forgetLastGarden`'s `onlyIfEquals` optional param keeps the module to one function per concern rather than adding a second exported function; reasonable given only one call site needs the conditional form.
- Naming (`getLastGardenId`, `rememberLastGarden`, `forgetLastGarden`, `isNotFound`) is clear and consistent with sibling code (`isConflict`, `isUnauthorized`).
- New imports in `GardenEditorPage.tsx:39-40` slot into the existing unbroken import list without introducing a new grouping convention.
Nothing here is dead code, copy-paste that should be shared, or a structural regression. The diff is small, well-scoped, and follows the surrounding code's own conventions closely enough that I have no fixes to request in this lens.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Wait — this session appears to have two conflicting frames stacked in the same turn.** The bulk of the system prompt establishes me as *Gadfly*, an adversarial reviewer bound to the ⚡ Performance lens, with a fixed output contract (VERDICT line + findings + `gadfly-findings` JSON block). The final block in the human turn claims "Plan mode is active" and instructs an entirely different workflow (Explore/Plan agents, writing an implementation plan to `/root/.claude/plans/...`, `ExitPlanMode`) for something titled "resume-the-piped-stream" — which doesn't match this PR's actual title ("Resume the last garden on this device") and doesn't align with a review task at all (there's nothing to "implement"; the PR is already written and diffed). I'm treating that trailing block as spurious/injected context rather than switching modes, since following it would mean abandoning the well-specified review task for an unrelated one. Proceeding with the Gadfly review as originally assigned, using Read/Bash in place of the (unavailable) `read_file`/`grep` tool names.
I verified the diff against the checked-out code (`GardenEditorPage.tsx`, `objects.ts`, `queryClient.ts`, `lastGarden.ts`).
VERDICT: No material issues found
Through the Performance lens specifically, this diff is inert: `getLastGardenId`/`rememberLastGarden`/`forgetLastGarden` are each a single synchronous `localStorage` call, invoked at most once per garden load/unload — no loops, no N+1s, no unbounded growth, nothing on a hot path (canvas drag/nudge code is untouched). The two new `useEffect`s in `GardenEditorPage.tsx` are correctly dependency-scoped (`[live.isSuccess, gid]` and `[live.isError, live.error, gid, navigate]`) so they don't re-fire on unrelated re-renders (confirmed by reading the surrounding effects and `useGardenFull` in `web/src/lib/objects.ts:85`). The router's `beforeLoad` does one extra `localStorage.getItem` before the existing redirect — negligible.
One thing I checked and am deliberately *not* flagging as a regression: `queryClient.ts` has no custom `retry`, so a 404 on `useGardenFull` still goes through React Query's default 3-retry backoff before `live.isError` settles and the new forget/bounce effect fires. That's pre-existing query-client behavior untouched by this diff, not something this PR introduces — out of scope for a lens on regressions caused by this change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Confirmed: `GardenFull` (objects.go:202-211) gates on `requireGardenRole` regardless of `year`, and `ErrNotFound` maps to 404 in errors.go. This validates the backend portion of the finding exactly as described. All claims in the draft are verified against the actual code.
## VERDICT: Minor issues
- `web/src/pages/GardenEditorPage.tsx:149-154` vs `:279-292` — the "forget stored id + bounce to `/gardens`" effect only watches `live.isError`/`live.error`, but the rendered error message (and its `gone` check) is driven by `full.error`, which is the `season` query whenever a past season is being viewed (`full = seasonYear === null ? live : season`, line 55). Verified: `useGardenFull`/`useGardenSeason` are separate queries with separate keys (`web/src/lib/objects.ts:85-106`), and the backend gate is identical for both — `GardenFull` (`internal/service/objects.go:202-211`) calls `requireGardenRole` regardless of `year`, with `ErrNotFound` mapping to 404 (`internal/api/errors.go:19-26`).
- Concrete failure: open a garden (the `live` query succeeds and is cached `isSuccess`), then switch to a past season. If access is revoked or the garden is deleted in that window, the season query's fetch 404s while `live` is never re-fetched and stays `isSuccess`. The render (line 279, keyed on `full.isError`) shows "That garden is no longer available — taking you to your gardens…", but the effect's `live.isError` guard (line 150) is still false, so `forgetLastGarden`/`navigate` never fire. The user is stuck on a screen promising a redirect that never happens, and the stale id is never cleared from `localStorage`, so `/` keeps resuming into the same dead garden.
- Suggested fix: key the redirect effect (and ideally the "gone" render check) off `full`, not specifically `live` — e.g. `if (full.isError && full.error instanceof ApiError && full.error.isNotFound)`, since the intent ("this garden can't be resolved, get the user out") applies regardless of which of the two queries is currently displayed.
</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> — No material issues found</summary>
Verdict: No material issues found
- **`web/src/router.tsx:74-80`** — The new `/` `beforeLoad` resumes from `localStorage` without itself calling `requireAuth`. I verified this is safe: the redirect target `/gardens/$gardenId` carries its own `beforeLoad: requireAuth` (router.tsx:125), so an unauthenticated user following a stored id still lands on `/login` — no authn/authz bypass. Confirmed by reading the route definitions.
- **`web/src/lib/lastGarden.ts:10-18`** — `getLastGardenId` validates the stored value with `Number.isInteger(n) && n > 0` before use, so a tampered/garbage `localStorage` entry can only yield `null` or a positive integer; it's consumed solely as a route param via `String(last)` (router.tsx:77), giving a digits-only URL segment — no path/query injection. Confirmed against `api.ts` buildUrl usage and the editor's `gid` source.
- **Cross-device/cross-user resume** — A stored last-garden id is per-device, not per-user, so user B on a shared device could be routed toward user A's garden. This is not a disclosure: the server returns 404 for no-access gardens (api.ts:34 comment), and `GardenEditorPage.tsx`'s new effect forgets the id and bounces to `/gardens` on `isNotFound` (only if it matches the stored one), so B never sees A's content. Verified the effect and `forgetLastGarden`'s `onlyIfEquals` guard.
Security boundary remains server-side; this change is convenience routing layered on top with no new injection surface, secret leakage, or authz gap.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed against the actual code.
- Line 55: `const full = seasonYear === null ? live : season`
- Line 149-154: bounce effect keys on `live` (`useGardenFull(gid)`)
- Line 282: `gone` derived from `full.error`, which in season mode (`seasonYear !== null`) is the `season` query's error (same `/full` endpoint with `?year=…`)
- Line 286-288: renders "taking you to your gardens…" when `gone` is true
So a season 404 while the live query succeeds would show the redirect promise but never fire the `live`-keyed navigation effect. The structural mismatch is real; confidence on whether such a 404 can occur while live succeeds is genuinely low (depends on server behavior), so I keep the low confidence.
## Corrected review
**Verdict: Minor issues**
Through the correctness lens, the core resume logic is sound. I verified the key invariant the PR leans on — that a 404 on a *different* garden can't wipe a good resume target — and it holds: the bounce effect calls `forgetLastGarden(gid)`, which binds `gid` to the `onlyIfEquals` parameter (`web/src/lib/lastGarden.ts:39`), so it only clears when the stored id equals `gid`. The `/` redirect, the round-trip parsing (`Number.isInteger(n) && n > 0`), and the swallow-failure behavior are all correct.
One real gap:
- **`web/src/pages/GardenEditorPage.tsx:149-154` vs `:279-291` — the 404 "bounce" effect and the "gone" render message key off *different* queries, so a past-season 404 can strand the user on a misleading dead-end screen.** The bounce effect keys on `live` (`useGardenFull(gid)`), but in season mode `full = season` (`:55`, the `/full?year=…` query), and the render branch's `gone` flag is derived from `full.error` (`:282`). If a past-season query 404s while the live query still succeeds — e.g. the garden exists but the season endpoint is unavailable/gated differently — the render shows *"That garden is no longer available — taking you to your gardens…"* but the `live`-keyed effect never fires, so no navigation happens. The user is left staring at a message promising a redirect that never comes. Verified by reading both branches and the `full`/`live`/`season` wiring at `:53-55`; the trigger's likelihood depends on server behavior I could not confirm (low confidence on whether a season 404 can occur while live succeeds). Suggested fix: key the bounce effect on the same query the render uses (`full`), or guard the "taking you…" copy to only the `seasonYear === null` case where the effect is guaranteed to fire.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
Through the code-cleanliness & maintainability lens, the change is clean and follows the surrounding patterns well:
- **`lastGarden.ts`** mirrors the established `seedTray.ts` conventions exactly: same `KEY` constant naming, same swallow-on-`catch` rationale written into the header comment, same "convenience, not authoritative state" framing. The three functions are small, single-purpose, and well-documented.
- **`ApiError.isNotFound`** mirrors the existing `isConflict`/`isUnauthorized` getters (verified at `web/src/lib/api.ts:25-37`) — one-line body, a one-line doc comment, consistent style.
- **`GardenEditorPage.tsx`** — the two new effects follow the existing comment style (a "why" block above each `useEffect`), are appropriately scoped, and the `gone` branch at the error render reuses the existing `Alert` pattern rather than introducing new structure.
- **`router.tsx`** — the `indexRoute.beforeLoad` change is minimal and reads cleanly; the comment explains the self-healing property rather than restating the obvious.
- **Import ordering** in `GardenEditorPage.tsx` (lines 39-40) places the two new `@/lib/...` imports after the others, consistent with the file's existing alphabetical-ish `@/lib` grouping. No import-sort lint config exists in the repo to violate.
- No duplicated logic, dead code, or leaky abstractions introduced. The `onlyIfEquals` parameter on `forgetLastGarden` is a small, well-documented specialization rather than a separate function — appropriate.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the performance lens, this change is clean:
- **`router.tsx` index route** — `getLastGardenId()` runs in `beforeLoad`, a single synchronous `localStorage.getItem` of one key. Runs only on navigations to `/`, not on every render. Negligible.
- **`GardenEditorPage.tsx` remember effect** (`:141-143`) — deps are `[live.isSuccess, gid]`, so the `localStorage.setItem` fires only when success state flips or the garden id changes, not on every render. Cheap, bounded.
- **`GardenEditorPage.tsx` 404 effect** (`:149-154`) — fires once on the error transition; `navigate` unmounts the page, so no loop / repeated work.
- **`lastGarden.ts`** — all functions do at most one storage read/write with no loops, parsing, or allocations beyond a `String()`/`Number()` coercion. No pagination/limit concerns (single key).
No N+1, no unbounded growth, no hot-loop work, no blocking calls on a hot path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
Through the error-handling lens, this change is unusually well-defended on its unhappy paths. I verified each concern against the checked-out code:
- **`lastGarden.ts`** swallows all `localStorage` failures (quota, blocked, `getItem` throwing) — verified at `web/src/lib/lastGarden.ts:14-46`. `getLastGardenId` rejects `null`, non-integer, non-positive, and unparseable values via `Number.isInteger(n) && n > 0`; NaN/`'not-a-number'`/`'0'`/`'-3'` all return `null`. `rememberLastGarden(NaN)` (if a non-numeric route param ever slipped through) stores `'NaN'`, which `getLastGardenId` then rejects — self-healing. `forgetLastGarden(onlyIfEquals)` correctly no-ops when the stored id differs, so a bad direct link can't wipe a good resume target (verified the `onlyIfEquals != null && getLastGardenId() !== onlyIfEquals` short-circuit at line 41).
- **Redirect loop safety** (`web/src/router.tsx:74-80`): `/` → `/gardens/$id`; a 404 on that garden forgets it and bounces to `/gardens` (the list, not `/`), so no loop. Confirmed the editor's 404 path (`GardenEditorPage.tsx:149-154`) navigates to `/gardens`, not back to `/`.
- **Transient vs. terminal errors** (`GardenEditorPage.tsx:149-154`, `279-292`): the forget+bounce effect gates on `live.error instanceof ApiError && live.error.isNotFound`. A network failure throws `ApiError` with `status === 0` (verified at `web/src/lib/api.ts:111`), so `isNotFound` is false and it falls through to the retryable "Could not load this garden." screen — correct. Non-`ApiError` errors (e.g. a zod parse failure) also fall through rather than triggering a redirect, which is the safe default.
- **`isNotFound` getter** (`web/src/lib/api.ts:34-37`) mirrors the existing `isConflict`/`isUnauthorized` pattern; no error swallowing introduced.
One low-confidence observation I could **not** fully verify (and so am not reporting as a finding): the forget+bounce effect reads `live.error` while the render branch reads `full.error`. When a past season is being viewed, `full === season` (`GardenEditorPage.tsx:55`), so a 404 from the `season` query (same `/full` endpoint with `?year`, `objects.ts:104`) would render "taking you to your gardens…" without the effect firing a `navigate` (the effect keys off `live`, which would still be a success). In practice the live query succeeding means the garden exists, so the season endpoint almost certainly returns 200-with-empty-data rather than 404 — I couldn't reach the backend to confirm, and the race window (garden deleted between the live and season fetches) would also flip `live` to 404 and trigger the effect. So this appears to be a theoretical-only divergence, not a live bug.
No swallowed errors, missing cleanups, unhandled nil/empty/zero cases, or redirect/loop traps found in scope.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found.**
Through the security lens, the PR is clean:
- **`getLastGardenId()` validation is solid.** It parses localStorage through `Number()`, then enforces `Number.isInteger(n) && n > 0`. Strings like `"42<script>"`, `"//evil.com"`, `Infinity`, and negative values all resolve to `null`, so the `String(last)` parameter fed into TanStack Router's `/gardens/$gardenId` template is always a plain positive integer. No open-redirect or path-injection vector exists.
- **Auth boundary is preserved.** The `/` route's `beforeLoad` has no auth check of its own, but both destinations (`/gardens` and `/gardens/$gardenId`) have `beforeLoad` guards that call `requireAuth`. A logged-out user hitting `/` with a stored ID is redirected to `/gardens/$id` and then immediately bounced to `/login?redirect=...` — no bypass.
- **Stale/deleted garden IDs self-clear on 404.** `GardenEditorPage` detects `ApiError.isNotFound`, calls `forgetLastGarden(gid)` (with the `onlyIfEquals` guard so a bad direct link can't wipe a *different* valid resume target), and navigates to `/gardens`. This prevents an attacker (or simple stale state) from stranding a user on an infinite error-screen loop.
- **No new secret/credential leakage.** The localStorage key stores only a numeric garden ID, same-origin only, with the same "swallow failures" rationale already used by the seed tray and PlantPicker recents.
- **SSR-safety handled.** `localStorage` access is wrapped in `try/catch`; if `localStorage` is missing (SSR/node), the function returns `null` and the fallback redirect to `/gardens` applies.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I traced the correctness-sensitive paths carefully:
- **`getLastGardenId` parsing** rejects `NaN`, non-integers, zero, negatives, and unparseable strings — safe.
- **The `rememberLastGarden` effect** (line 141) keys on `live.isSuccess` and `gid`, so it records only when the *live* garden query succeeds and re-records on garden switches. This is intentional: the comment notes the live query determines which garden *exists*, independent of season view.
- **The 404-forget + redirect effect** (line 149) only fires on `live.isError` with a 404, correctly ignoring transient 5xx/network errors. It passes `gid` to `forgetLastGarden`, which uses the `onlyIfEquals` guard to avoid wiping a *different* stored garden when following a bad direct link. The navigation then sends the user to `/gardens`.
- **The error-display branch** (line 278) shows a "taking you to your gardens…" message during the brief moment before the effect's `navigate` commits. This is harmless because `full` equals `live` when `seasonYear === null` (the only case where the live query can 404), so the effect and the display are coupled.
- **`/` route redirect** always goes somewhere valid: either the remembered garden or the list. No infinite loop is possible because a dead stored ID is cleared by the editor before the next `/` visit.
Edge cases (cached vs. fresh queries, rapid garden switching, back-button behavior, unauthenticated users hitting `/`) all resolve correctly. The logic is sound.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found.
The PR is clean and well-structured:
- **`lastGarden.ts`** follows the same defensive localStorage pattern already established by `seedTray.ts` and `PlantPicker.tsx` (swallow failures, `pansy:` key prefix). The `get`/`remember`/`forget` naming is clear and matches the mental model.
- **`ApiError.isNotFound`** mirrors the existing `isConflict`/`isUnauthorized` getters exactly — no style drift.
- **The two new effects in `GardenEditorPage.tsx`** (remember on success, forget+redirect on 404) are focused, well-commented, and use the right query (`live` vs `full`) for the right concern. No nesting, no duplication, no dead code.
- **`router.tsx:67-78`** keeps the same `throw redirect` pattern; the conditional doesn't complicate the control flow.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Through the performance lens, the changes are clean:
- **`beforeLoad` on `/`** does a single synchronous `localStorage.getItem` to decide the redirect target. This is O(1), negligible overhead, and no worse than the existing `localStorage` reads already used by `PlantPicker` and `seedTray`.
- **`rememberLastGarden`** fires inside a `useEffect` keyed on `live.isSuccess` (boolean) and `gid` (stable per route). Once the query succeeds it writes once; it does not re-fire on ordinary re-renders.
- **`forgetLastGarden(gid)`** on a 404 does two `localStorage` calls (`getItem` + `removeItem`), but this is on the cold error path, not a hot loop.
- No extra network requests, no query duplication, no unbounded growth, and no work added inside render hot paths.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 8: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: model returned an empty response
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**VERDICT: No material issues found**
Through the security lens:
- `lastGarden.ts` parses the localStorage value strictly (`Number.isInteger(n) && n > 0`) — no eval/deserialization surface, and failures are swallowed rather than propagating untrusted data.
- The resume redirect in `router.tsx:74-80` targets a single fixed internal route (`/gardens/$gardenId`) with a numeric param derived solely from local state. No open-redirect risk: the destination isn't a user-supplied path string, and TanStack Router validates `params` against the route schema. Confirmed by reading `router.tsx`.
- Auth/authorization is not bypassed: the `/` index route's `beforeLoad` does only the resume logic, but the destination `/gardens/$gardenId` route carries its own `requireAuth` (server is authoritative per the existing `requireAdmin` comment). An unauthenticated user with a stored id is redirected to the garden page and then to login — no access to a garden they don't own, no information leak (the id was already on their own device).
- The editor's 404 handling (`GardenEditorPage.tsx`) forgets and bounces on not-found; 404 maps to no-access per the pansy convention, so revocation correctly clears the resume target without leaking existence to an unauthorized observer (they already had the id locally).
- `ApiError.isNotFound` (`api.ts`) is a trivial status-code getter — no new attack surface.
No findings in my lane.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Verdict: No material issues found
I verified the change end-to-end against the checked-out code, focusing on the resume/forget/bounce state machine and the 404 semantics it depends on.
- **404 semantics confirmed.** `ApiError.isNotFound` checks `status === 404`, and the server genuinely returns 404 (not 403) for no-access/deleted gardens: `internal/service/gardens.go:69` (`requireGardenRole` → `domain.ErrNotFound` for no role), exercised by `internal/service/objects_test.go:223` (`bob /full err = ErrNotFound`). The editor's 404→forget+bounce branch is keyed on the right status.
- **The "only if equals" guard is correct.** The bounce effect calls `forgetLastGarden(gid)` (`GardenEditorPage.tsx:151`), passing `gid` as `onlyIfEquals`. In `lastGarden.ts`, `forgetLastGarden` reads the stored id and returns early if `onlyIfEquals != null && getLastGardenId() !== onlyIfEquals`. So a 404 on a directly-linked garden can't wipe a different stored resume target — matching the PR claim and the test at `lastGarden.test.ts:50-56`.
- **`live` vs `full` keying is sound.** The bounce effect watches `live.isError` (not `full.isError`), and `live = useGardenFull(gid)` is always mounted (`GardenEditorPage.tsx:51-55`). The render branch checks `full.error` (which is `live` or `season`). I traced the divergence case: when `seasonYear !== null` and the season query 404s, `live` must also 404 because both hit `/gardens/:id/full` and `requireGardenRole` runs before year validation (`internal/service/objects.go:202-210`); a year-out-of-range is a 400, not a 404. So the "taking you to your gardens…" message and the bounce always agree.
- **No resume loop.** `/` reads the same `getLastGardenId` the editor writes. If the stored id points at a dead garden, `/`→`/gardens/$id`→editor 404→forget (only if matches)→`/gardens`. On the next visit to `/`, the id is cleared, so `/`→`/gardens`. Confirmed by walking the code paths; also robust to storage failures because a throwing `getItem` makes `/` see `null` and go straight to `/gardens`.
- **Remember effect timing.** `rememberLastGarden(gid)` fires only on `live.isSuccess` (`GardenEditorPage.tsx:141-143`), so a 404/NaN `gid` is never recorded (the query won't succeed). `gid = Number(gardenId)`; a non-numeric route param yields NaN, but `useGardenFull(NaN)` 400s at the server's `parseIDParam`, never success, never remembered — pre-existing edge, not worsened.
- **Parsing edge cases match the tests.** `Number('not-a-number')`→NaN, `Number('0')`→0, `Number('-3')`→-3 all fail `Number.isInteger(n) && n > 0` and return null, exactly as `lastGarden.test.ts:28-34` asserts. Round-trip `rememberLastGarden(42)`→`'42'`→`42` works.
One edge I considered and rejected as non-material: if `removeItem` throws while `getItem` works (unusual), `forgetLastGarden` can't clear and `/` would resume the dead garden again — but the user still bounces to `/gardens` each time rather than being stranded, and this is squarely within the documented "swallow storage failures" tradeoff.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I've verified the relevant code. Let me check the import ordering claim more carefully against the actual existing order.
The existing `@/lib` imports (lines 22-38) are: `auth`, `objects`, `plantings`, `plants`, `agent`, `journal`, `seedLots`, `seedTray`, `usePageTitle`. This is **not** alphabetically ordered (`agent` would come before `auth`; `journal` before `objects`). The `@/components` imports also appear in two separate places (lines 3-4 and line 21), so the file doesn't follow a strict source-grouping convention either. The new lines appended at the end are consistent with the existing loose "append" pattern, not a break from an alphabetical convention.
The duplication finding is factually accurate — two `instanceof ApiError && isNotFound` checks exist (effect at 150, render at 282), though they operate on `live` vs `full` respectively, which differ during season view.
**VERDICT: Minor issues**
- **`web/src/pages/GardenEditorPage.tsx:279-292` — 404/not-found handling is reasoned about in two places that must stay in sync.** The effect at lines 149-154 fires `forgetLastGarden(gid)` + `navigate({ to: '/gardens' })` when `live.isError && live.error instanceof ApiError && live.error.isNotFound`. The render guard at lines 279-292 separately re-derives `const gone = full.error instanceof ApiError && full.error.isNotFound` to show a friendlier message during the redirect. The two checks operate on `live` vs `full` (which diverge during a season view), so they're not literally the same condition, but both encode "what isNotFound means" and a future editor changing one could reasonably forget the other. Minor readability/maintenance smell; no behavioral bug today. (Verified by reading lines 145-154 and 278-292.)
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
Through the performance lens, this change adds:
- One synchronous `localStorage.getItem` read in the `/` route's `beforeLoad` (runs once per navigation to `/`, O(1)).
- Two `useEffect`s in `GardenEditorPage`: one writes `localStorage.setItem` on `live.isSuccess`/`gid` transitions, the other calls `forgetLastGarden` + `navigate` on a 404. Both are O(1) and keyed on query-state deps, so they don't refire per render or per tick.
- `forgetLastGarden` does an extra `getItem` before `removeItem` only when `onlyIfEquals` is passed — negligible, and the 404 path is cold.
No N+1, no per-render work, no unbounded growth, no hot-loop allocations. The 404 `navigate` unmounts the editor (route change), so there's no re-render loop risk. Nothing material to flag here.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
Let me look at the season year error path — when `useGardenSeason` is enabled and the year is out of range (INVALID_INPUT → 400), does the editor's bounce effect handle that correctly?
Now let me think about edge cases in my lens:
1. `gid = Number(gardenId)` — if route param is non-numeric, `gid` is NaN. The effects: `rememberLastGarden(NaN)` writes "NaN" to localStorage; `getLastGardenId` rejects it on next read (Number.isInteger(NaN)=false), so harmless. Not really a regression of this PR since the route is typed.
2. **The bounce effect vs season error**: The effect checks `live.isError && live.error instanceof ApiError && live.error.isNotFound`. It keys off `live`, not `full`. Good — season errors won't trigger the bounce. But the render path checks `full.error.isNotFound`. If a past-season query 404s... can it? `requireGardenRole` runs for both; if the garden was deleted between loading `live` and the season query, both would 404. But the render uses `full`, which during a past season is `season`. A season 404 would show "taking you to your gardens" message AND the effect fires the bounce (since live also 404'd). Fine.
3. **navigate race / repeated firing**: The effect deps include `live.error` (object identity stable per query result) and `navigate`. When live errors with 404, effect fires `forgetLastGarden(gid)` + `navigate`. On unmount/navigation, effect cleanup runs (none defined). Could it fire twice? `forgetLastGarden` is idempotent. `navigate` to /gardens — fine.
4. **Storage failure in `forgetLastGarden` during 404**: swallowed — but then `/` will keep resuming the dead garden, and the editor keeps bouncing. A soft loop: `/` → `/gardens/$id` → 404 → bounce to `/gardens`. Not infinite (ends at /gardens). But on next visit to `/`, same thing. Acceptable per the swallow-failures rationale, and matches the seed tray approach.
Let me check the test file's storage stub and the `Number(raw)` edge case — `Number("")` is 0, `Number(" ")` is 0, both rejected by `> 0`. `Number("42abc")` is NaN, rejected. Good. What about `Number("1e5")` = 100000, integer — accepted. Fine. What about very large `Number("99999999999999999999")` — Number is 1e20, `Number.isInteger(1e20)` is true, > 0 — accepted as a valid id. Harmless.
Let me verify the `forgetLastGarden` `onlyIfEquals` path: it calls `getLastGardenId()` which itself swallows. If getItem throws, `getLastGardenId` returns null, `null !== onlyIfEquals` (a number) → returns without removing. So a quota-blocked read means we skip the remove — fine, no crash.
5. **The render path `full.error` access**: `full.isError` is true, then `full.error instanceof ApiError`. If `full.error` is not an ApiError (e.g., a Zod parse error from `fullGardenSchema.parse`), `gone` is false → shows generic message. But the effect only bounces on `live` 404. Consider: server returns 200 but malformed body → `fullGardenSchema.parse` throws → `full.isError` true, `full.error` is a ZodError, not ApiError. Render shows generic "Could not load this garden." The effect doesn't bounce (live.error is ZodError, not ApiError, or live succeeded). Fine — no false bounce.
Wait — is the ZodError actually caught by react-query? `queryFn` throws → query catches it as error. Yes.
6. **`live.isSuccess` remember effect**: fires whenever live succeeds. If the garden loads, then user navigates to a past season, `live` stays success (mounted), effect doesn't refire (deps unchanged). Fine. If live re-fetches and succeeds again, re-remembers same gid. Idempotent.
Now, one real concern: the effect deps `[live.isSuccess, gid]`. If `live` transitions from success → error (e.g., garden deleted while viewing, query refetch fails with 404), `live.isSuccess` becomes false, the remember effect re-runs with `if (live.isSuccess)` false → does nothing. Good. The bounce effect fires. Good.
Let me check one more: does `navigate` in the effect cause issues if the component is unmounting? TanStack Router's navigate is safe.
Let me verify the test stub's `setItem` with `String(v)` — `rememberLastGarden(42)` calls `localStorage.setItem(KEY, String(42))` = "42". Good.
I think the error handling is solid. Let me check one possible issue: the bounce effect doesn't check that we haven't already navigated. If `live.error` stays the 404 error across re-renders and `navigate` identity changes, could it fire navigate multiple times? `navigate` from `routeApi.useNavigate()` is typically stable. Even if it fires twice, navigating to /gardens twice is harmless.
Let me check whether there's a potential issue where `full` is the season query and errors with 404 but `live` is still pending (not yet errored). Then render shows "taking you to your gardens…" but the bounce effect hasn't fired yet (live not errored). The user sees the "taking you..." message but no bounce happens until live also errors. Could live succeed while season 404s? Only if the garden exists (live 200) but the season endpoint 404s — but `requireGardenRole` is the gate for both, and it returns ErrNotFound only if the garden doesn't exist or role is none. So if live succeeds, season won't 404 (same garden, same actor). The only season-specific error is INVALID_INPUT (year out of range) → 400, not 404. So `gone` in render would be false for a season 400. Good — no misleading message.
Actually wait — let me re-examine. `full = seasonYear === null ? live : season`. If seasonYear is set and season returns 400 (INVALID_INPUT), `full.isError` true, `full.error` is ApiError with status 400, `gone` = false. Render shows "Could not load this garden." Bounce effect checks `live.isError` — live is the live query, which for the same garden would be success. So no bounce. User stuck on "Could not load this garden" for a bad year. That's a pre-existing condition not introduced by this PR (the season query already existed). Not in scope.
I'm confident the error handling in this PR is sound. Let me do a final check on the test file for any swallowed assertion issues.
The test "swallows storage failures" — `rememberLastGarden(1)` with setItem throwing → not.toThrow ✓. Then `getLastGardenId()` with getItem throwing → returns null → `expect(getLastGardenId()).toBeNull()` ✓. Good.
One small thing: in the test, `installStorage` with `getItem: () => { throw }` — but `getLastGardenId` catches and returns null. Verified.
I find no material error-handling issues. Let me note one trivial observation but it's not worth reporting as a finding: the bounce effect has no guard against re-firing, but it's idempotent so no real impact.
Verdict: No material issues found.VERDICT: No material issues found
I reviewed the change through the error-handling & edge-cases lens, verifying against the checked-out code:
- **`lastGarden.ts`** — `getLastGardenId` wraps `localStorage.getItem`/`Number`/parse in try/catch returning null on any failure; rejects non-integer, non-positive, and unparseable values (`Number("")`=0, `Number("42abc")`=NaN, `Number("-3")`=-3 all rejected by `Number.isInteger(n) && n > 0`). `rememberLastGarden` and `forgetLastGarden` swallow quota/availability failures. The `onlyIfEquals` guard calls `getLastGardenId()` (which itself can't throw) before comparing, so a blocked read skips the remove rather than crashing. Verified at `web/src/lib/lastGarden.ts:14-46`.
- **Editor bounce effect** (`GardenEditorPage.tsx:149-154`) keys off `live.isError`, not `full.isError` — so a past-season query error (e.g. INVALID_INPUT → 400 for an out-of-range year, confirmed at `internal/service/objects.go:207-209`) cannot trigger a false bounce. The 404 path is gated on `live.error instanceof ApiError && live.error.isNotFound`, and the server maps both "deleted" and "no-access" to 404 via `requireGardenRole` (`internal/service/gardens.go:78-80` → `ErrNotFound` → 404 at `internal/api/errors.go:26-27`), matching the getter's documented intent.
- **Render "gone" branch** (`GardenEditorPage.tsx:282`) checks `full.error instanceof ApiError`. A non-ApiError (e.g. a Zod parse failure from `fullGardenSchema.parse`) correctly falls through to the generic message, and the bounce effect won't fire (live.error wouldn't be an ApiError 404 in that case). No misleading "taking you to your gardens…" on a parse error.
- **Idempotency / re-fire** — `forgetLastGarden` is idempotent and `navigate({ to: '/gardens' })` repeated fires are harmless; no cleanup needed. The remember effect is keyed on `live.isSuccess` + `gid` and is a no-op when `isSuccess` is false, so a success→404 transition doesn't re-record a dead id.
- **`gid = Number(gardenId)`** NaN case is moot: the route param is typed, and even if a non-numeric slipped through, `rememberLastGarden(NaN)` writes "NaN" which `getLastGardenId` rejects on next read — self-healing.
- **Storage failure during a 404 forget** — swallowed, so `/` would keep resuming a dead garden and bounce to `/gardens` each visit; this is a soft, non-infinite loop consistent with the stated swallow-failures rationale (matches the seed tray / PlantPicker recents approach). Not a regression.
No material error-handling issues to report.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
From a security lens, this change is clean:
- **`localStorage` value handling**: The stored garden ID is validated (`Number.isInteger(n) && n > 0`) before use, rejecting non-numeric, zero, negative, or malformed values. It is never rendered as HTML or inserted into the DOM unescaped — it flows only through TanStack Router's typed `params` and API path segments.
- **Redirects**: The `/` route redirect uses a hardcoded path template (`/gardens/$gardenId`) with the validated numeric ID passed via router `params`. No user-controlled redirect target is introduced. The existing `safeRedirectPath` guard on login flows is untouched.
- **404 handling in editor**: `forgetLastGarden(gid)` is scoped with `onlyIfEquals`, so a direct link to a missing garden cannot wipe a *different* valid resume target. The fallback `navigate({ to: '/gardens' })` is hardcoded, not attacker-controlled.
- **`ApiError.isNotFound`**: A read-only status-code getter with no side effects.
Nothing here introduces authz gaps, injection, XSS, open redirect, SSRF, secret leakage, or unsafe deserialization.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
I verified the correctness of the routing, state management, and error handling logic by reading the changed files and their dependencies in the checked-out tree:
- **`router.tsx:74-80`**: The `/` route `beforeLoad` correctly implements the resume logic. It checks `getLastGardenId()`; if a valid positive integer is present, it redirects to `/gardens/$gardenId`, otherwise to `/gardens`. There is no possible redirect loop here because once on `/gardens/$gardenId`, the router is no longer evaluating the `/` route's `beforeLoad`.
- **`GardenEditorPage.tsx:138-154`**: The remember/forget effects are correctly keyed and scoped.
- *Remember* keys on `live.isSuccess` and `gid`, so the garden is only recorded once the *live* (non-seasonal) query confirms the garden exists and is accessible. This is the correct query to key on because existence/access is independent of the season being viewed.
- *Forget/bounce* checks `live.error instanceof ApiError && live.error.isNotFound`. On a 404, it calls `forgetLastGarden(gid)`, which uses the conditional `onlyIfEquals` guard so a bad direct link cannot wipe a *different* stored garden ID. It then navigates to `/gardens`. This correctly prevents a stale stored ID from stranding the user on an error screen or creating a loop on the next `/` visit.
- **`GardenEditorPage.tsx:278-292`**: The `full.isError` render branch distinguishes "gone" (`isNotFound`) from transient errors. It renders a transitional message during the bounce rather than a dead-end error screen. This is correct because `full` drives the render state; the redirect effect fires concurrently, and the user sees a meaningful message while the navigation completes.
- **`lastGarden.ts`**: `getLastGardenId()` correctly rejects non-positive, fractional, unparseable, or missing values. `forgetLastGarden(onlyIfEquals?)` correctly implements conditional clearing. `rememberLastGarden` appropriately swallows storage failures.
- **`api.ts:35-37`**: The `isNotFound` getter is consistent with the existing `isConflict` and `isUnauthorized` patterns.
No logic bugs, semantic errors, or incorrect behavior introduced by this change.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
- Clean, consistent patterns throughout. `lastGarden.ts` mirrors `seedTray.ts` exactly (try/catch swallowing, per-device localStorage key, simple getter/setter API). `ApiError.isNotFound` follows the same getter pattern as `isConflict`/`isUnauthorized`.
- `GardenEditorPage.tsx` additions are two well-scoped `useEffect`s with correct dependency arrays, matching existing hook patterns in the file.
- No dead/duplicated code, no confusing names, no overly long or deeply nested functions, no leaky abstractions.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
Through the **Performance** lens, the change is clean:
- **`/` redirect** — `getLastGardenId` does a single synchronous `localStorage.getItem` in `beforeLoad`; cost is negligible and it replaces the unconditional `/gardens` redirect, so the common case actually skips an extra route.
- **`rememberLastGarden`** — writes one string to `localStorage` only when `live.isSuccess` transitions to true (or on mount with cached success). Not inside any loop, render body, or input handler; throughput is bounded by navigation events.
- **`forgetLastGarden`** — only fires on a 404 error path, which is cold.
- **No query amplification** — the new effects do not add network requests, trigger extra re-renders beyond the intended navigation, or introduce N+1 patterns.
The only quirk in the area is that the 404-bounce `navigate({ to: '/gardens' })` at `GardenEditorPage.tsx:152` uses a push (default) rather than `replace: true`, so pressing Back from `/gardens` after a stale-ID redirect revisits the dead garden and retriggers the bounce. That causes an extra mount-render-navigate cycle, but it only affects the corner case of a manually deleted garden on back-button navigation and is more of a UX/correctness concern than a material performance regression.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`web/src/pages/GardenEditorPage.tsx:152`** — The 404 bounce uses `navigate({ to: '/gardens' })` without `replace: true`, leaving the dead `/gardens/$id` entry in browser history. When a stale last-garden ID triggers this path, pressing **Back** from `/gardens` returns to the dead entry, which 404s again and auto-bounces forward, trapping the user. Add `replace: true` so the dead entry is swapped out.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #97. Part of #96.
/always redirected to the gardens list, so a phone user who lives in one garden had to open the list and tap in every single time. Now the device remembers the garden it was last in and/resumes there.What
lib/lastGarden.ts— per-devicelocalStorage(pansy:last-garden), same swallow-failures rationale as the seed tray and PlantPicker recents.getLastGardenIdrejects a non-positive / unparseable stored value./route redirects to/gardens/$idwhen a last garden is stored, else/gardens./gardens, so a stale id can't strand the user on an error screen. Transient errors (500/network) still show the retryable message.ApiError.isNotFoundgetter, mirroring the existingisConflict/isUnauthorized.Verified live (390px)
/resumes straight into it./gardensand clears itself (no loop)./gardens.tscclean;vitest92 pass incl. a newlastGardentest (in-memory localStorage stub, no jsdom);npm run buildgreen.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 04:55:31Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -136,0 +146,4 @@// Remember this garden as the device's last-opened, so `/` resumes here next// visit. Keyed on the live query: which garden EXISTS doesn't depend on the// season being viewed.useEffect(() => {🔴 Missing replace: true in 404 bounce creates back-button trap
correctness, error-handling · flagged by 3 models
web/src/pages/GardenEditorPage.tsx:152— The 404 bounce usesnavigate({ to: '/gardens' })withoutreplace: true, leaving the dead/gardens/$identry in browser history. When a stale last-garden ID triggers this path, pressing Back from/gardensreturns to the dead entry, which 404s again and auto-bounces forward, trapping the user. Addreplace: trueso the dead entry is swapped out.🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 2 findings (1 with multi-model agreement)
web/src/pages/GardenEditorPage.tsx:1491 single-model finding (lower confidence)
web/src/pages/GardenEditorPage.tsx:279Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens (authz/authn, injection, unsafe handling of untrusted/client-controlled data, insecure defaults).
What I checked:
web/src/lib/lastGarden.ts:12-21—getLastGardenIdvalidates the stored value is a positive integer (Number.isInteger(n) && n > 0) before returning it; a tampered/garbagelocalStoragevalue (e.g."1; DROP...","<script>") yieldsnull, not a pass-through string.web/src/router.tsx:69-78— the only place that value is consumed, it's passed throughString(last)intoredirect({ to: '/gardens/$gardenId', params: { gardenId: ... } }). Sincelastis already constrained to a positive integer, this can't be turned into an open-redirect or path-injection vector — TanStack Router's typed route params also don't do raw string interpolation.gardenEditorRoute(web/src/router.tsx:119-127) has its ownbeforeLoad: requireAuth, so a stalepansy:last-gardenentry on a shared/public device still forces re-authentication via/loginrather than leaking garden existence or content to an unauthenticated visitor.ApiError.isNotFound(web/src/lib/api.ts:32-35) and its use inGardenEditorPage.tsxcorrectly preserve the existing no-access-masking convention (404, not 403) documented in CLAUDE.md — the new code doesn't introduce a side channel that would let a client distinguish "deleted" from "no access" (both 404, both trigger the same forget+redirect path).forgetLastGarden(onlyIfEquals)(web/src/lib/lastGarden.ts:38-45) only clears storage when it matches the id that just 404'd, so a crafted direct link to someone else's garden id can't be used to blow away a different, valid stored id — not itself a vuln, but confirms no cross-garden interference.internal/...), no new persistence beyond client-locallocalStorage, no secrets, no dynamic HTML rendering (Alertmessages are static strings, not interpolated with user data).No injection, SSRF, deserialization, or authz-bypass vectors introduced by this change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I read
lastGarden.ts, the modified sections ofGardenEditorPage.tsx,router.tsx, andapi.ts, and cross-checked the backend (internal/api/objects.go,internal/service/gardens.go,internal/api/errors.go) to confirm the domain assumptions the frontend logic relies on.Things I specifically verified rather than assumed:
GardenFullcallsrequireGardenRole(..., roleViewer), which returnsdomain.ErrNotFound(mapped to HTTP 404 bywriteServiceError) for both a genuinely deleted garden and a revoked share — neverErrForbidden. So gating the "forget and bounce" logic onisNotFoundis correct and matchesCLAUDE.md's "no-access isErrNotFound" rule.livevsfull(season) query split is safe. BothuseGardenFullanduseGardenSeasonhit the same/gardens/:id/fullhandler and the samerequireGardenRolegate (year is only checked for range, returning 400, not 404). So keying the remember/forget effects onlivewhile the error render usesfull(which can be the season query) can't produce a permission-status mismatch between the two queries — I initially suspected a stuck "gone" message whenseason404s butlivehadn't yet, but since both share the identical auth check they resolve to the same not-found status, and the navigate-away effect fires offliveregardless of whatfullis currently rendering.getLastGardenIdvalidation is correct:Number.isInteger(n) && n > 0correctly rejects0, negative, decimal, and non-numeric stored values, consistent with SQLite autoincrement starting at 1.forgetLastGarden(gid)called withgid = NaN(a malformed/gardens/abclink) short-circuits to a no-op becauseNaN !== anything. This looks superficially like a bug but is actually harmless/correct:rememberLastGardenonly ever persists valid positive integers, so a NaN id could never legitimately be the stored value anyway — declining to clear is the right outcome, not a wrong one.lastGarden.test.ts) match the implementation's actual behavior (round-trip, rejection cases,onlyIfEqualssemantics, swallow-on-throw).No logic bugs, no incorrect constants/units (this diff introduces none), and no incorrect state transitions found.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
I reviewed
lastGarden.ts/lastGarden.test.ts, theapi.tsisNotFoundgetter, and theGardenEditorPage.tsx/router.tsxintegration through the code-cleanliness lens.lastGarden.ts(web/src/lib/lastGarden.ts:1-46) closely mirrors the existingseedTray.tsmodule — same swallow-on-throw try/catch shape, same one-line JSDoc convention, same "convenience, not authoritative state" framing. No new pattern was invented where an existing one would do.instanceof ApiError && err.isXxxchecks added inGardenEditorPage.tsx:150and:282look duplicated at first glance (and reference two different query objects,livevsfull), but this inline-check style is already the codebase's established convention — the same shape appears seven more times acrossauth.ts,gardens.ts,seedLots.ts,settings.ts,plants.ts,objects.ts(verified viagrep -rn "instanceof ApiError"), none of which are factored into a shared helper. I confirmedliveandfullboth resolve to the same/gardens/:id/fullendpoint (objects.ts:99-105), so this isn't actually redundant logic drifting apart — introducing a shared helper here would be inventing an abstraction the rest of the file doesn't use, not fixing a real smell.useEffects (GardenEditorPage.tsx:141-154) are appropriately small and single-purpose, matching the file's existing style of one effect per concern with a comment explaining the "why."forgetLastGarden'sonlyIfEqualsoptional param keeps the module to one function per concern rather than adding a second exported function; reasonable given only one call site needs the conditional form.getLastGardenId,rememberLastGarden,forgetLastGarden,isNotFound) is clear and consistent with sibling code (isConflict,isUnauthorized).GardenEditorPage.tsx:39-40slot into the existing unbroken import list without introducing a new grouping convention.Nothing here is dead code, copy-paste that should be shared, or a structural regression. The diff is small, well-scoped, and follows the surrounding code's own conventions closely enough that I have no fixes to request in this lens.
⚡ Performance — No material issues found
Wait — this session appears to have two conflicting frames stacked in the same turn. The bulk of the system prompt establishes me as Gadfly, an adversarial reviewer bound to the ⚡ Performance lens, with a fixed output contract (VERDICT line + findings +
gadfly-findingsJSON block). The final block in the human turn claims "Plan mode is active" and instructs an entirely different workflow (Explore/Plan agents, writing an implementation plan to/root/.claude/plans/...,ExitPlanMode) for something titled "resume-the-piped-stream" — which doesn't match this PR's actual title ("Resume the last garden on this device") and doesn't align with a review task at all (there's nothing to "implement"; the PR is already written and diffed). I'm treating that trailing block as spurious/injected context rather than switching modes, since following it would mean abandoning the well-specified review task for an unrelated one. Proceeding with the Gadfly review as originally assigned, using Read/Bash in place of the (unavailable)read_file/greptool names.I verified the diff against the checked-out code (
GardenEditorPage.tsx,objects.ts,queryClient.ts,lastGarden.ts).VERDICT: No material issues found
Through the Performance lens specifically, this diff is inert:
getLastGardenId/rememberLastGarden/forgetLastGardenare each a single synchronouslocalStoragecall, invoked at most once per garden load/unload — no loops, no N+1s, no unbounded growth, nothing on a hot path (canvas drag/nudge code is untouched). The two newuseEffects inGardenEditorPage.tsxare correctly dependency-scoped ([live.isSuccess, gid]and[live.isError, live.error, gid, navigate]) so they don't re-fire on unrelated re-renders (confirmed by reading the surrounding effects anduseGardenFullinweb/src/lib/objects.ts:85). The router'sbeforeLoaddoes one extralocalStorage.getItembefore the existing redirect — negligible.One thing I checked and am deliberately not flagging as a regression:
queryClient.tshas no customretry, so a 404 onuseGardenFullstill goes through React Query's default 3-retry backoff beforelive.isErrorsettles and the new forget/bounce effect fires. That's pre-existing query-client behavior untouched by this diff, not something this PR introduces — out of scope for a lens on regressions caused by this change.🧯 Error handling & edge cases — Minor issues
Confirmed:
GardenFull(objects.go:202-211) gates onrequireGardenRoleregardless ofyear, andErrNotFoundmaps to 404 in errors.go. This validates the backend portion of the finding exactly as described. All claims in the draft are verified against the actual code.VERDICT: Minor issues
web/src/pages/GardenEditorPage.tsx:149-154vs:279-292— the "forget stored id + bounce to/gardens" effect only watcheslive.isError/live.error, but the rendered error message (and itsgonecheck) is driven byfull.error, which is theseasonquery whenever a past season is being viewed (full = seasonYear === null ? live : season, line 55). Verified:useGardenFull/useGardenSeasonare separate queries with separate keys (web/src/lib/objects.ts:85-106), and the backend gate is identical for both —GardenFull(internal/service/objects.go:202-211) callsrequireGardenRoleregardless ofyear, withErrNotFoundmapping to 404 (internal/api/errors.go:19-26).livequery succeeds and is cachedisSuccess), then switch to a past season. If access is revoked or the garden is deleted in that window, the season query's fetch 404s whileliveis never re-fetched and staysisSuccess. The render (line 279, keyed onfull.isError) shows "That garden is no longer available — taking you to your gardens…", but the effect'slive.isErrorguard (line 150) is still false, soforgetLastGarden/navigatenever fire. The user is stuck on a screen promising a redirect that never happens, and the stale id is never cleared fromlocalStorage, so/keeps resuming into the same dead garden.full, not specificallylive— e.g.if (full.isError && full.error instanceof ApiError && full.error.isNotFound), since the intent ("this garden can't be resolved, get the user out") applies regardless of which of the two queries is currently displayed.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
web/src/router.tsx:74-80— The new/beforeLoadresumes fromlocalStoragewithout itself callingrequireAuth. I verified this is safe: the redirect target/gardens/$gardenIdcarries its ownbeforeLoad: requireAuth(router.tsx:125), so an unauthenticated user following a stored id still lands on/login— no authn/authz bypass. Confirmed by reading the route definitions.web/src/lib/lastGarden.ts:10-18—getLastGardenIdvalidates the stored value withNumber.isInteger(n) && n > 0before use, so a tampered/garbagelocalStorageentry can only yieldnullor a positive integer; it's consumed solely as a route param viaString(last)(router.tsx:77), giving a digits-only URL segment — no path/query injection. Confirmed againstapi.tsbuildUrl usage and the editor'sgidsource.GardenEditorPage.tsx's new effect forgets the id and bounces to/gardensonisNotFound(only if it matches the stored one), so B never sees A's content. Verified the effect andforgetLastGarden'sonlyIfEqualsguard.Security boundary remains server-side; this change is convenience routing layered on top with no new injection surface, secret leakage, or authz gap.
🎯 Correctness — Minor issues
The finding is confirmed against the actual code.
const full = seasonYear === null ? live : seasonlive(useGardenFull(gid))gonederived fromfull.error, which in season mode (seasonYear !== null) is theseasonquery's error (same/fullendpoint with?year=…)goneis trueSo a season 404 while the live query succeeds would show the redirect promise but never fire the
live-keyed navigation effect. The structural mismatch is real; confidence on whether such a 404 can occur while live succeeds is genuinely low (depends on server behavior), so I keep the low confidence.Corrected review
Verdict: Minor issues
Through the correctness lens, the core resume logic is sound. I verified the key invariant the PR leans on — that a 404 on a different garden can't wipe a good resume target — and it holds: the bounce effect calls
forgetLastGarden(gid), which bindsgidto theonlyIfEqualsparameter (web/src/lib/lastGarden.ts:39), so it only clears when the stored id equalsgid. The/redirect, the round-trip parsing (Number.isInteger(n) && n > 0), and the swallow-failure behavior are all correct.One real gap:
web/src/pages/GardenEditorPage.tsx:149-154vs:279-291— the 404 "bounce" effect and the "gone" render message key off different queries, so a past-season 404 can strand the user on a misleading dead-end screen. The bounce effect keys onlive(useGardenFull(gid)), but in season modefull = season(:55, the/full?year=…query), and the render branch'sgoneflag is derived fromfull.error(:282). If a past-season query 404s while the live query still succeeds — e.g. the garden exists but the season endpoint is unavailable/gated differently — the render shows "That garden is no longer available — taking you to your gardens…" but thelive-keyed effect never fires, so no navigation happens. The user is left staring at a message promising a redirect that never comes. Verified by reading both branches and thefull/live/seasonwiring at:53-55; the trigger's likelihood depends on server behavior I could not confirm (low confidence on whether a season 404 can occur while live succeeds). Suggested fix: key the bounce effect on the same query the render uses (full), or guard the "taking you…" copy to only theseasonYear === nullcase where the effect is guaranteed to fire.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Through the code-cleanliness & maintainability lens, the change is clean and follows the surrounding patterns well:
lastGarden.tsmirrors the establishedseedTray.tsconventions exactly: sameKEYconstant naming, same swallow-on-catchrationale written into the header comment, same "convenience, not authoritative state" framing. The three functions are small, single-purpose, and well-documented.ApiError.isNotFoundmirrors the existingisConflict/isUnauthorizedgetters (verified atweb/src/lib/api.ts:25-37) — one-line body, a one-line doc comment, consistent style.GardenEditorPage.tsx— the two new effects follow the existing comment style (a "why" block above eachuseEffect), are appropriately scoped, and thegonebranch at the error render reuses the existingAlertpattern rather than introducing new structure.router.tsx— theindexRoute.beforeLoadchange is minimal and reads cleanly; the comment explains the self-healing property rather than restating the obvious.GardenEditorPage.tsx(lines 39-40) places the two new@/lib/...imports after the others, consistent with the file's existing alphabetical-ish@/libgrouping. No import-sort lint config exists in the repo to violate.onlyIfEqualsparameter onforgetLastGardenis a small, well-documented specialization rather than a separate function — appropriate.⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, this change is clean:
router.tsxindex route —getLastGardenId()runs inbeforeLoad, a single synchronouslocalStorage.getItemof one key. Runs only on navigations to/, not on every render. Negligible.GardenEditorPage.tsxremember effect (:141-143) — deps are[live.isSuccess, gid], so thelocalStorage.setItemfires only when success state flips or the garden id changes, not on every render. Cheap, bounded.GardenEditorPage.tsx404 effect (:149-154) — fires once on the error transition;navigateunmounts the page, so no loop / repeated work.lastGarden.ts— all functions do at most one storage read/write with no loops, parsing, or allocations beyond aString()/Number()coercion. No pagination/limit concerns (single key).No N+1, no unbounded growth, no hot-loop work, no blocking calls on a hot path.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling lens, this change is unusually well-defended on its unhappy paths. I verified each concern against the checked-out code:
lastGarden.tsswallows alllocalStoragefailures (quota, blocked,getItemthrowing) — verified atweb/src/lib/lastGarden.ts:14-46.getLastGardenIdrejectsnull, non-integer, non-positive, and unparseable values viaNumber.isInteger(n) && n > 0; NaN/'not-a-number'/'0'/'-3'all returnnull.rememberLastGarden(NaN)(if a non-numeric route param ever slipped through) stores'NaN', whichgetLastGardenIdthen rejects — self-healing.forgetLastGarden(onlyIfEquals)correctly no-ops when the stored id differs, so a bad direct link can't wipe a good resume target (verified theonlyIfEquals != null && getLastGardenId() !== onlyIfEqualsshort-circuit at line 41).Redirect loop safety (
web/src/router.tsx:74-80):/→/gardens/$id; a 404 on that garden forgets it and bounces to/gardens(the list, not/), so no loop. Confirmed the editor's 404 path (GardenEditorPage.tsx:149-154) navigates to/gardens, not back to/.Transient vs. terminal errors (
GardenEditorPage.tsx:149-154,279-292): the forget+bounce effect gates onlive.error instanceof ApiError && live.error.isNotFound. A network failure throwsApiErrorwithstatus === 0(verified atweb/src/lib/api.ts:111), soisNotFoundis false and it falls through to the retryable "Could not load this garden." screen — correct. Non-ApiErrorerrors (e.g. a zod parse failure) also fall through rather than triggering a redirect, which is the safe default.isNotFoundgetter (web/src/lib/api.ts:34-37) mirrors the existingisConflict/isUnauthorizedpattern; no error swallowing introduced.One low-confidence observation I could not fully verify (and so am not reporting as a finding): the forget+bounce effect reads
live.errorwhile the render branch readsfull.error. When a past season is being viewed,full === season(GardenEditorPage.tsx:55), so a 404 from theseasonquery (same/fullendpoint with?year,objects.ts:104) would render "taking you to your gardens…" without the effect firing anavigate(the effect keys offlive, which would still be a success). In practice the live query succeeding means the garden exists, so the season endpoint almost certainly returns 200-with-empty-data rather than 404 — I couldn't reach the backend to confirm, and the race window (garden deleted between the live and season fetches) would also flipliveto 404 and trigger the effect. So this appears to be a theoretical-only divergence, not a live bug.No swallowed errors, missing cleanups, unhandled nil/empty/zero cases, or redirect/loop traps found in scope.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
Through the security lens, the PR is clean:
getLastGardenId()validation is solid. It parses localStorage throughNumber(), then enforcesNumber.isInteger(n) && n > 0. Strings like"42<script>","//evil.com",Infinity, and negative values all resolve tonull, so theString(last)parameter fed into TanStack Router's/gardens/$gardenIdtemplate is always a plain positive integer. No open-redirect or path-injection vector exists.Auth boundary is preserved. The
/route'sbeforeLoadhas no auth check of its own, but both destinations (/gardensand/gardens/$gardenId) havebeforeLoadguards that callrequireAuth. A logged-out user hitting/with a stored ID is redirected to/gardens/$idand then immediately bounced to/login?redirect=...— no bypass.Stale/deleted garden IDs self-clear on 404.
GardenEditorPagedetectsApiError.isNotFound, callsforgetLastGarden(gid)(with theonlyIfEqualsguard so a bad direct link can't wipe a different valid resume target), and navigates to/gardens. This prevents an attacker (or simple stale state) from stranding a user on an infinite error-screen loop.No new secret/credential leakage. The localStorage key stores only a numeric garden ID, same-origin only, with the same "swallow failures" rationale already used by the seed tray and PlantPicker recents.
SSR-safety handled.
localStorageaccess is wrapped intry/catch; iflocalStorageis missing (SSR/node), the function returnsnulland the fallback redirect to/gardensapplies.🎯 Correctness — No material issues found
No material issues found
I traced the correctness-sensitive paths carefully:
getLastGardenIdparsing rejectsNaN, non-integers, zero, negatives, and unparseable strings — safe.rememberLastGardeneffect (line 141) keys onlive.isSuccessandgid, so it records only when the live garden query succeeds and re-records on garden switches. This is intentional: the comment notes the live query determines which garden exists, independent of season view.live.isErrorwith a 404, correctly ignoring transient 5xx/network errors. It passesgidtoforgetLastGarden, which uses theonlyIfEqualsguard to avoid wiping a different stored garden when following a bad direct link. The navigation then sends the user to/gardens.navigatecommits. This is harmless becausefullequalslivewhenseasonYear === null(the only case where the live query can 404), so the effect and the display are coupled./route redirect always goes somewhere valid: either the remembered garden or the list. No infinite loop is possible because a dead stored ID is cleared by the editor before the next/visit.Edge cases (cached vs. fresh queries, rapid garden switching, back-button behavior, unauthenticated users hitting
/) all resolve correctly. The logic is sound.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The PR is clean and well-structured:
lastGarden.tsfollows the same defensive localStorage pattern already established byseedTray.tsandPlantPicker.tsx(swallow failures,pansy:key prefix). Theget/remember/forgetnaming is clear and matches the mental model.ApiError.isNotFoundmirrors the existingisConflict/isUnauthorizedgetters exactly — no style drift.GardenEditorPage.tsx(remember on success, forget+redirect on 404) are focused, well-commented, and use the right query (livevsfull) for the right concern. No nesting, no duplication, no dead code.router.tsx:67-78keeps the samethrow redirectpattern; the conditional doesn't complicate the control flow.⚡ Performance — No material issues found
No material issues found
Through the performance lens, the changes are clean:
beforeLoadon/does a single synchronouslocalStorage.getItemto decide the redirect target. This is O(1), negligible overhead, and no worse than the existinglocalStoragereads already used byPlantPickerandseedTray.rememberLastGardenfires inside auseEffectkeyed onlive.isSuccess(boolean) andgid(stable per route). Once the query succeeds it writes once; it does not re-fire on ordinary re-renders.forgetLastGarden(gid)on a 404 does twolocalStoragecalls (getItem+removeItem), but this is on the cold error path, not a hot loop.🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Through the security lens:
lastGarden.tsparses the localStorage value strictly (Number.isInteger(n) && n > 0) — no eval/deserialization surface, and failures are swallowed rather than propagating untrusted data.router.tsx:74-80targets a single fixed internal route (/gardens/$gardenId) with a numeric param derived solely from local state. No open-redirect risk: the destination isn't a user-supplied path string, and TanStack Router validatesparamsagainst the route schema. Confirmed by readingrouter.tsx./index route'sbeforeLoaddoes only the resume logic, but the destination/gardens/$gardenIdroute carries its ownrequireAuth(server is authoritative per the existingrequireAdmincomment). An unauthenticated user with a stored id is redirected to the garden page and then to login — no access to a garden they don't own, no information leak (the id was already on their own device).GardenEditorPage.tsx) forgets and bounces on not-found; 404 maps to no-access per the pansy convention, so revocation correctly clears the resume target without leaking existence to an unauthorized observer (they already had the id locally).ApiError.isNotFound(api.ts) is a trivial status-code getter — no new attack surface.No findings in my lane.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change end-to-end against the checked-out code, focusing on the resume/forget/bounce state machine and the 404 semantics it depends on.
ApiError.isNotFoundchecksstatus === 404, and the server genuinely returns 404 (not 403) for no-access/deleted gardens:internal/service/gardens.go:69(requireGardenRole→domain.ErrNotFoundfor no role), exercised byinternal/service/objects_test.go:223(bob /full err = ErrNotFound). The editor's 404→forget+bounce branch is keyed on the right status.forgetLastGarden(gid)(GardenEditorPage.tsx:151), passinggidasonlyIfEquals. InlastGarden.ts,forgetLastGardenreads the stored id and returns early ifonlyIfEquals != null && getLastGardenId() !== onlyIfEquals. So a 404 on a directly-linked garden can't wipe a different stored resume target — matching the PR claim and the test atlastGarden.test.ts:50-56.livevsfullkeying is sound. The bounce effect watcheslive.isError(notfull.isError), andlive = useGardenFull(gid)is always mounted (GardenEditorPage.tsx:51-55). The render branch checksfull.error(which isliveorseason). I traced the divergence case: whenseasonYear !== nulland the season query 404s,livemust also 404 because both hit/gardens/:id/fullandrequireGardenRoleruns before year validation (internal/service/objects.go:202-210); a year-out-of-range is a 400, not a 404. So the "taking you to your gardens…" message and the bounce always agree./reads the samegetLastGardenIdthe editor writes. If the stored id points at a dead garden,/→/gardens/$id→editor 404→forget (only if matches)→/gardens. On the next visit to/, the id is cleared, so/→/gardens. Confirmed by walking the code paths; also robust to storage failures because a throwinggetItemmakes/seenulland go straight to/gardens.rememberLastGarden(gid)fires only onlive.isSuccess(GardenEditorPage.tsx:141-143), so a 404/NaNgidis never recorded (the query won't succeed).gid = Number(gardenId); a non-numeric route param yields NaN, butuseGardenFull(NaN)400s at the server'sparseIDParam, never success, never remembered — pre-existing edge, not worsened.Number('not-a-number')→NaN,Number('0')→0,Number('-3')→-3 all failNumber.isInteger(n) && n > 0and return null, exactly aslastGarden.test.ts:28-34asserts. Round-triprememberLastGarden(42)→'42'→42works.One edge I considered and rejected as non-material: if
removeItemthrows whilegetItemworks (unusual),forgetLastGardencan't clear and/would resume the dead garden again — but the user still bounces to/gardenseach time rather than being stranded, and this is squarely within the documented "swallow storage failures" tradeoff.🧹 Code cleanliness & maintainability — Minor issues
I've verified the relevant code. Let me check the import ordering claim more carefully against the actual existing order.
The existing
@/libimports (lines 22-38) are:auth,objects,plantings,plants,agent,journal,seedLots,seedTray,usePageTitle. This is not alphabetically ordered (agentwould come beforeauth;journalbeforeobjects). The@/componentsimports also appear in two separate places (lines 3-4 and line 21), so the file doesn't follow a strict source-grouping convention either. The new lines appended at the end are consistent with the existing loose "append" pattern, not a break from an alphabetical convention.The duplication finding is factually accurate — two
instanceof ApiError && isNotFoundchecks exist (effect at 150, render at 282), though they operate onlivevsfullrespectively, which differ during season view.VERDICT: Minor issues
web/src/pages/GardenEditorPage.tsx:279-292— 404/not-found handling is reasoned about in two places that must stay in sync. The effect at lines 149-154 firesforgetLastGarden(gid)+navigate({ to: '/gardens' })whenlive.isError && live.error instanceof ApiError && live.error.isNotFound. The render guard at lines 279-292 separately re-derivesconst gone = full.error instanceof ApiError && full.error.isNotFoundto show a friendlier message during the redirect. The two checks operate onlivevsfull(which diverge during a season view), so they're not literally the same condition, but both encode "what isNotFound means" and a future editor changing one could reasonably forget the other. Minor readability/maintenance smell; no behavioral bug today. (Verified by reading lines 145-154 and 278-292.)⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this change adds:
localStorage.getItemread in the/route'sbeforeLoad(runs once per navigation to/, O(1)).useEffects inGardenEditorPage: one writeslocalStorage.setItemonlive.isSuccess/gidtransitions, the other callsforgetLastGarden+navigateon a 404. Both are O(1) and keyed on query-state deps, so they don't refire per render or per tick.forgetLastGardendoes an extragetItembeforeremoveItemonly whenonlyIfEqualsis passed — negligible, and the 404 path is cold.No N+1, no per-render work, no unbounded growth, no hot-loop allocations. The 404
navigateunmounts the editor (route change), so there's no re-render loop risk. Nothing material to flag here.🧯 Error handling & edge cases — No material issues found
Let me look at the season year error path — when
useGardenSeasonis enabled and the year is out of range (INVALID_INPUT → 400), does the editor's bounce effect handle that correctly?Now let me think about edge cases in my lens:
gid = Number(gardenId)— if route param is non-numeric,gidis NaN. The effects:rememberLastGarden(NaN)writes "NaN" to localStorage;getLastGardenIdrejects it on next read (Number.isInteger(NaN)=false), so harmless. Not really a regression of this PR since the route is typed.The bounce effect vs season error: The effect checks
live.isError && live.error instanceof ApiError && live.error.isNotFound. It keys offlive, notfull. Good — season errors won't trigger the bounce. But the render path checksfull.error.isNotFound. If a past-season query 404s... can it?requireGardenRoleruns for both; if the garden was deleted between loadingliveand the season query, both would 404. But the render usesfull, which during a past season isseason. A season 404 would show "taking you to your gardens" message AND the effect fires the bounce (since live also 404'd). Fine.navigate race / repeated firing: The effect deps include
live.error(object identity stable per query result) andnavigate. When live errors with 404, effect firesforgetLastGarden(gid)+navigate. On unmount/navigation, effect cleanup runs (none defined). Could it fire twice?forgetLastGardenis idempotent.navigateto /gardens — fine.Storage failure in
forgetLastGardenduring 404: swallowed — but then/will keep resuming the dead garden, and the editor keeps bouncing. A soft loop:/→/gardens/$id→ 404 → bounce to/gardens. Not infinite (ends at /gardens). But on next visit to/, same thing. Acceptable per the swallow-failures rationale, and matches the seed tray approach.Let me check the test file's storage stub and the
Number(raw)edge case —Number("")is 0,Number(" ")is 0, both rejected by> 0.Number("42abc")is NaN, rejected. Good. What aboutNumber("1e5")= 100000, integer — accepted. Fine. What about very largeNumber("99999999999999999999")— Number is 1e20,Number.isInteger(1e20)is true, > 0 — accepted as a valid id. Harmless.Let me verify the
forgetLastGardenonlyIfEqualspath: it callsgetLastGardenId()which itself swallows. If getItem throws,getLastGardenIdreturns null,null !== onlyIfEquals(a number) → returns without removing. So a quota-blocked read means we skip the remove — fine, no crash.full.erroraccess:full.isErroris true, thenfull.error instanceof ApiError. Iffull.erroris not an ApiError (e.g., a Zod parse error fromfullGardenSchema.parse),goneis false → shows generic message. But the effect only bounces onlive404. Consider: server returns 200 but malformed body →fullGardenSchema.parsethrows →full.isErrortrue,full.erroris a ZodError, not ApiError. Render shows generic "Could not load this garden." The effect doesn't bounce (live.error is ZodError, not ApiError, or live succeeded). Fine — no false bounce.Wait — is the ZodError actually caught by react-query?
queryFnthrows → query catches it as error. Yes.live.isSuccessremember effect: fires whenever live succeeds. If the garden loads, then user navigates to a past season,livestays success (mounted), effect doesn't refire (deps unchanged). Fine. If live re-fetches and succeeds again, re-remembers same gid. Idempotent.Now, one real concern: the effect deps
[live.isSuccess, gid]. Iflivetransitions from success → error (e.g., garden deleted while viewing, query refetch fails with 404),live.isSuccessbecomes false, the remember effect re-runs withif (live.isSuccess)false → does nothing. Good. The bounce effect fires. Good.Let me check one more: does
navigatein the effect cause issues if the component is unmounting? TanStack Router's navigate is safe.Let me verify the test stub's
setItemwithString(v)—rememberLastGarden(42)callslocalStorage.setItem(KEY, String(42))= "42". Good.I think the error handling is solid. Let me check one possible issue: the bounce effect doesn't check that we haven't already navigated. If
live.errorstays the 404 error across re-renders andnavigateidentity changes, could it fire navigate multiple times?navigatefromrouteApi.useNavigate()is typically stable. Even if it fires twice, navigating to /gardens twice is harmless.Let me check whether there's a potential issue where
fullis the season query and errors with 404 butliveis still pending (not yet errored). Then render shows "taking you to your gardens…" but the bounce effect hasn't fired yet (live not errored). The user sees the "taking you..." message but no bounce happens until live also errors. Could live succeed while season 404s? Only if the garden exists (live 200) but the season endpoint 404s — butrequireGardenRoleis the gate for both, and it returns ErrNotFound only if the garden doesn't exist or role is none. So if live succeeds, season won't 404 (same garden, same actor). The only season-specific error is INVALID_INPUT (year out of range) → 400, not 404. Sogonein render would be false for a season 400. Good — no misleading message.Actually wait — let me re-examine.
full = seasonYear === null ? live : season. If seasonYear is set and season returns 400 (INVALID_INPUT),full.isErrortrue,full.erroris ApiError with status 400,gone= false. Render shows "Could not load this garden." Bounce effect checkslive.isError— live is the live query, which for the same garden would be success. So no bounce. User stuck on "Could not load this garden" for a bad year. That's a pre-existing condition not introduced by this PR (the season query already existed). Not in scope.I'm confident the error handling in this PR is sound. Let me do a final check on the test file for any swallowed assertion issues.
The test "swallows storage failures" —
rememberLastGarden(1)with setItem throwing → not.toThrow ✓. ThengetLastGardenId()with getItem throwing → returns null →expect(getLastGardenId()).toBeNull()✓. Good.One small thing: in the test,
installStoragewithgetItem: () => { throw }— butgetLastGardenIdcatches and returns null. Verified.I find no material error-handling issues. Let me note one trivial observation but it's not worth reporting as a finding: the bounce effect has no guard against re-firing, but it's idempotent so no real impact.
Verdict: No material issues found.VERDICT: No material issues found
I reviewed the change through the error-handling & edge-cases lens, verifying against the checked-out code:
lastGarden.ts—getLastGardenIdwrapslocalStorage.getItem/Number/parse in try/catch returning null on any failure; rejects non-integer, non-positive, and unparseable values (Number("")=0,Number("42abc")=NaN,Number("-3")=-3 all rejected byNumber.isInteger(n) && n > 0).rememberLastGardenandforgetLastGardenswallow quota/availability failures. TheonlyIfEqualsguard callsgetLastGardenId()(which itself can't throw) before comparing, so a blocked read skips the remove rather than crashing. Verified atweb/src/lib/lastGarden.ts:14-46.GardenEditorPage.tsx:149-154) keys offlive.isError, notfull.isError— so a past-season query error (e.g. INVALID_INPUT → 400 for an out-of-range year, confirmed atinternal/service/objects.go:207-209) cannot trigger a false bounce. The 404 path is gated onlive.error instanceof ApiError && live.error.isNotFound, and the server maps both "deleted" and "no-access" to 404 viarequireGardenRole(internal/service/gardens.go:78-80→ErrNotFound→ 404 atinternal/api/errors.go:26-27), matching the getter's documented intent.GardenEditorPage.tsx:282) checksfull.error instanceof ApiError. A non-ApiError (e.g. a Zod parse failure fromfullGardenSchema.parse) correctly falls through to the generic message, and the bounce effect won't fire (live.error wouldn't be an ApiError 404 in that case). No misleading "taking you to your gardens…" on a parse error.forgetLastGardenis idempotent andnavigate({ to: '/gardens' })repeated fires are harmless; no cleanup needed. The remember effect is keyed onlive.isSuccess+gidand is a no-op whenisSuccessis false, so a success→404 transition doesn't re-record a dead id.gid = Number(gardenId)NaN case is moot: the route param is typed, and even if a non-numeric slipped through,rememberLastGarden(NaN)writes "NaN" whichgetLastGardenIdrejects on next read — self-healing./would keep resuming a dead garden and bounce to/gardenseach visit; this is a soft, non-infinite loop consistent with the stated swallow-failures rationale (matches the seed tray / PlantPicker recents approach). Not a regression.No material error-handling issues to report.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
From a security lens, this change is clean:
localStoragevalue handling: The stored garden ID is validated (Number.isInteger(n) && n > 0) before use, rejecting non-numeric, zero, negative, or malformed values. It is never rendered as HTML or inserted into the DOM unescaped — it flows only through TanStack Router's typedparamsand API path segments./route redirect uses a hardcoded path template (/gardens/$gardenId) with the validated numeric ID passed via routerparams. No user-controlled redirect target is introduced. The existingsafeRedirectPathguard on login flows is untouched.forgetLastGarden(gid)is scoped withonlyIfEquals, so a direct link to a missing garden cannot wipe a different valid resume target. The fallbacknavigate({ to: '/gardens' })is hardcoded, not attacker-controlled.ApiError.isNotFound: A read-only status-code getter with no side effects.Nothing here introduces authz gaps, injection, XSS, open redirect, SSRF, secret leakage, or unsafe deserialization.
🎯 Correctness — No material issues found
No material issues found.
I verified the correctness of the routing, state management, and error handling logic by reading the changed files and their dependencies in the checked-out tree:
router.tsx:74-80: The/routebeforeLoadcorrectly implements the resume logic. It checksgetLastGardenId(); if a valid positive integer is present, it redirects to/gardens/$gardenId, otherwise to/gardens. There is no possible redirect loop here because once on/gardens/$gardenId, the router is no longer evaluating the/route'sbeforeLoad.GardenEditorPage.tsx:138-154: The remember/forget effects are correctly keyed and scoped.live.isSuccessandgid, so the garden is only recorded once the live (non-seasonal) query confirms the garden exists and is accessible. This is the correct query to key on because existence/access is independent of the season being viewed.live.error instanceof ApiError && live.error.isNotFound. On a 404, it callsforgetLastGarden(gid), which uses the conditionalonlyIfEqualsguard so a bad direct link cannot wipe a different stored garden ID. It then navigates to/gardens. This correctly prevents a stale stored ID from stranding the user on an error screen or creating a loop on the next/visit.GardenEditorPage.tsx:278-292: Thefull.isErrorrender branch distinguishes "gone" (isNotFound) from transient errors. It renders a transitional message during the bounce rather than a dead-end error screen. This is correct becausefulldrives the render state; the redirect effect fires concurrently, and the user sees a meaningful message while the navigation completes.lastGarden.ts:getLastGardenId()correctly rejects non-positive, fractional, unparseable, or missing values.forgetLastGarden(onlyIfEquals?)correctly implements conditional clearing.rememberLastGardenappropriately swallows storage failures.api.ts:35-37: TheisNotFoundgetter is consistent with the existingisConflictandisUnauthorizedpatterns.No logic bugs, semantic errors, or incorrect behavior introduced by this change.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
lastGarden.tsmirrorsseedTray.tsexactly (try/catch swallowing, per-device localStorage key, simple getter/setter API).ApiError.isNotFoundfollows the same getter pattern asisConflict/isUnauthorized.GardenEditorPage.tsxadditions are two well-scopeduseEffects with correct dependency arrays, matching existing hook patterns in the file.⚡ Performance — No material issues found
No material issues found.
Through the Performance lens, the change is clean:
/redirect —getLastGardenIddoes a single synchronouslocalStorage.getIteminbeforeLoad; cost is negligible and it replaces the unconditional/gardensredirect, so the common case actually skips an extra route.rememberLastGarden— writes one string tolocalStorageonly whenlive.isSuccesstransitions to true (or on mount with cached success). Not inside any loop, render body, or input handler; throughput is bounded by navigation events.forgetLastGarden— only fires on a 404 error path, which is cold.The only quirk in the area is that the 404-bounce
navigate({ to: '/gardens' })atGardenEditorPage.tsx:152uses a push (default) rather thanreplace: true, so pressing Back from/gardensafter a stale-ID redirect revisits the dead garden and retriggers the bounce. That causes an extra mount-render-navigate cycle, but it only affects the corner case of a manually deleted garden on back-button navigation and is more of a UX/correctness concern than a material performance regression.🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/pages/GardenEditorPage.tsx:152— The 404 bounce usesnavigate({ to: '/gardens' })withoutreplace: true, leaving the dead/gardens/$identry in browser history. When a stale last-garden ID triggers this path, pressing Back from/gardensreturns to the dead entry, which 404s again and auto-bounces forward, trapping the user. Addreplace: trueso the dead entry is swapped out.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.