Rough edges: mobile 100dvh, session-expiry in chat, sticky error toasts, journal date filter #93

Merged
steve merged 2 commits from fix/rough-edges into main 2026-07-22 03:54:03 +00:00
Owner

Part of #85, part of #86. The safe, self-contained wins from the bundle.

  • Mobile viewport — the editor used 100vh, which on mobile Safari/Chrome is the largest viewport (URL bar hidden), so with the bar showing the canvas bottom + Fit button were pushed under the browser chrome. 100dvh.
  • Session expiry mid-chat — a 401 during a chat turn was reported as "the assistant is not available right now", sending the user to chase a config problem that isn't there. Now: "Your session has expired — please sign in again" + redirect to /login preserving the path, matching how the rest of the app treats 401 (ApiError.isUnauthorized).
  • Error toasts persist — errors are the primary report that a mutation failed, yet they auto-dismissed at 4s with no way back. Errors now stay until dismissed; info toasts still time out; both get a ✕ close button.
  • Journal date filterfrom/to existed in the API and JournalFilter but had no UI, so "show me last spring" was unreachable. Two date inputs (+ Clear) feed the existing filter. No backend change.

Deferred (noted, not done here)

Too big for a rough-edges bundle, or needs a call:

  • Revision-history retention/pruning — a real feature (migration + policy); its own issue.
  • Agent toolbox's missing corrective tools (resize/delete object, remove one plop, read journal, undo) — its own issue.
  • Plop-level journal entries from the UI — small, but a distinct addition.
  • Editor max-w cap — a layout call (the canvas is capped at 1024px on a big monitor). Worth confirming the intent rather than changing blind.

The DESIGN.md API-listing drift #85 mentioned was already fixed in #89.

tsc --noEmit and 87 vitest tests pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

Part of #85, part of #86. The safe, self-contained wins from the bundle. - **Mobile viewport** — the editor used `100vh`, which on mobile Safari/Chrome is the *largest* viewport (URL bar hidden), so with the bar showing the canvas bottom + Fit button were pushed under the browser chrome. `100dvh`. - **Session expiry mid-chat** — a 401 during a chat turn was reported as "the assistant is not available right now", sending the user to chase a config problem that isn't there. Now: "Your session has expired — please sign in again" + redirect to `/login` preserving the path, matching how the rest of the app treats 401 (`ApiError.isUnauthorized`). - **Error toasts persist** — errors are the primary report that a mutation failed, yet they auto-dismissed at 4s with no way back. Errors now stay until dismissed; info toasts still time out; both get a ✕ close button. - **Journal date filter** — `from`/`to` existed in the API and `JournalFilter` but had no UI, so "show me last spring" was unreachable. Two date inputs (+ Clear) feed the existing filter. **No backend change.** ## Deferred (noted, not done here) Too big for a rough-edges bundle, or needs a call: - **Revision-history retention/pruning** — a real feature (migration + policy); its own issue. - **Agent toolbox's missing corrective tools** (resize/delete object, remove one plop, read journal, undo) — its own issue. - **Plop-level journal entries from the UI** — small, but a distinct addition. - **Editor `max-w` cap** — a layout call (the canvas is capped at 1024px on a big monitor). Worth confirming the intent rather than changing blind. The DESIGN.md API-listing drift #85 mentioned was already fixed in #89. `tsc --noEmit` and 87 vitest tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve added 1 commit 2026-07-22 03:31:28 +00:00
Rough edges: mobile 100dvh, session-expiry in chat, sticky error toasts, journal date filter (#85)
Build image / build-and-push (push) Successful in 30s
Gadfly review (reusable) / review (pull_request) Successful in 5m41s
Adversarial Review (Gadfly) / review (pull_request) Successful in 5m41s
d82db48e4b
The safe, self-contained wins from the #85 bundle:

- **Mobile viewport.** The editor used `100vh`, which on mobile Safari/Chrome is
  the LARGEST viewport (URL bar hidden), so with the bar visible the canvas
  bottom and Fit button were pushed under the browser chrome. `100dvh` fixes it.
- **Session expiry mid-chat.** A 401 during a chat turn was mapped to "the
  assistant is not available right now" — sending the user to debug a config
  problem that isn't there. Now it says the session expired and redirects to
  /login, preserving the path, like the rest of the app treats 401.
- **Error toasts persist.** Error toasts were the primary report that a mutation
  failed, yet auto-dismissed at 4s with no way to retrieve them — look away and
  it's gone. Errors now stay until dismissed; info toasts still time out; both
  get a close button.
- **Journal date filter.** `from`/`to` existed in the API and JournalFilter but
  had no UI, so "show me last spring" was unreachable. Added two date inputs
  (with a Clear) that feed the existing filter. No backend change.

Deferred to their own follow-ups (too big for this bundle, or need a decision):
revision-history retention/pruning, the agent toolbox's missing corrective tools,
plop-level journal entries from the UI, and the editor's max-width cap (a layout
call worth confirming rather than changing blind). The DESIGN.md API-listing
drift the issue mentioned was already fixed in #89.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-22 03:37:11Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • 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 — No material issues found

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 — could not complete
  • performance — No material issues found
  • error-handling — No material issues found

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 03:37:11Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **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** — No material issues found #### `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** — could not complete - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-22 03:37:11 +00:00
gitea-actions bot left a comment

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

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -36,0 +37,4 @@
// Error toasts are the primary report that a mutation failed, so they do NOT
// auto-dismiss — a user who looked away at second 4 would otherwise lose the
// only notice, with nothing to retrieve (#85). Info toasts still time out.
if (isError) return

🟡 Error toasts no longer auto-dismiss and the store has no cap, so repeated errors grow the toast stack/DOM unbounded

performance · flagged by 1 model

  • web/src/components/ui/toast.tsx:40 — error toasts now accumulate with no bound. Previously every toast auto-dismissed at 4s, so the toasts array in the zustand store self-drained. Now error toasts never time out and are removed only by an explicit dismiss click. The store has no max-cap — push just appends (line 22: set((s) => ({ toasts: [...s.toasts, { id: nextId++, message, tone }] }))) — and Toaster renders the entire array (lines 73-75). Any path that fires repeated `toast.…

🪰 Gadfly · advisory

🟡 **Error toasts no longer auto-dismiss and the store has no cap, so repeated errors grow the toast stack/DOM unbounded** _performance · flagged by 1 model_ - **`web/src/components/ui/toast.tsx:40` — error toasts now accumulate with no bound.** Previously every toast auto-dismissed at 4s, so the `toasts` array in the zustand store self-drained. Now error toasts never time out and are removed only by an explicit `dismiss` click. The store has no max-cap — `push` just appends (line 22: `set((s) => ({ toasts: [...s.toasts, { id: nextId++, message, tone }] }))`) — and `Toaster` renders the entire array (lines 73-75). Any path that fires repeated `toast.… <sub>🪰 Gadfly · advisory</sub>
@@ -46,0 +46,4 @@
// from/to; they just had no UI. Empty inputs don't filter.
const [from, setFrom] = useState('')
const [to, setTo] = useState('')
const filter = {

filter built from three stacked ternary-spread branches; a compact helper or plain assignments would scale better as filter fields grow

maintainability · flagged by 1 model

  • web/src/editor/JournalPanel.tsx:49-53filter built from three stacked ternary-spread branches. The ...(scopeObjectId != null ? { objectId: scopeObjectId } : {}) idiom, stacked three times, reads as clever rather than clear. A compact/Object.fromEntries(Object.entries({...}).filter(...)) helper or plain assignments into a const filter: JournalFilter = {} would scale better as more filter fields appear. Trivial maintainability nit.

🪰 Gadfly · advisory

⚪ **filter built from three stacked ternary-spread branches; a compact helper or plain assignments would scale better as filter fields grow** _maintainability · flagged by 1 model_ - **`web/src/editor/JournalPanel.tsx:49-53` — `filter` built from three stacked ternary-spread branches.** The `...(scopeObjectId != null ? { objectId: scopeObjectId } : {})` idiom, stacked three times, reads as clever rather than clear. A `compact`/`Object.fromEntries(Object.entries({...}).filter(...))` helper or plain assignments into a `const filter: JournalFilter = {}` would scale better as more filter fields appear. Trivial maintainability nit. <sub>🪰 Gadfly · advisory</sub>
@@ -73,0 +86,4 @@
value={from}
max={to || undefined}
onChange={(e) => setFrom(e.target.value)}
className="rounded-md border border-border bg-surface px-1.5 py-1 text-fg outline-none focus-visible:ring-2 focus-visible:ring-accent/40"

🟡 Journal date filter inputs hand-roll styling instead of reusing shared fieldControlClass, dropping text-base (iOS zoom-on-focus) and diverging focus-ring styling from every other input in the app

maintainability · flagged by 2 models

  • web/src/editor/JournalPanel.tsx:89,99 — date-input control styling duplicated verbatim with no shared class constant. The rounded-md border border-border bg-surface px-1.5 py-1 text-fg outline-none focus-visible:ring-2 focus-visible:ring-accent/40 string is repeated identically for the From and To inputs (lines 89 and 99), and a near-identical variant already exists for the scope <select> at line 69 and the Clear button at line 109. A small const inputCls = '…' at module top…

🪰 Gadfly · advisory

🟡 **Journal date filter inputs hand-roll styling instead of reusing shared fieldControlClass, dropping text-base (iOS zoom-on-focus) and diverging focus-ring styling from every other input in the app** _maintainability · flagged by 2 models_ - **`web/src/editor/JournalPanel.tsx:89,99` — date-input control styling duplicated verbatim with no shared class constant.** The `rounded-md border border-border bg-surface px-1.5 py-1 text-fg outline-none focus-visible:ring-2 focus-visible:ring-accent/40` string is repeated identically for the `From` and `To` inputs (lines 89 and 99), and a near-identical variant already exists for the scope `<select>` at line 69 and the `Clear` button at line 109. A small `const inputCls = '…'` at module top… <sub>🪰 Gadfly · advisory</sub>
@@ -163,0 +165,4 @@
// unavailable" would send the user chasing a config problem that isn't there.
// Send them to sign in again, preserving where they were.
handlers.onError('Your session has expired — please sign in again.')
const back = encodeURIComponent(location.pathname + location.search)

🟡 401 redirect hand-rolls encodeURIComponent + window.location.assign instead of reusing safeRedirectPath for the value construction used elsewhere for auth redirects

maintainability · flagged by 1 model

  • web/src/lib/agent.ts:168-169 — 401 redirect hand-rolls the login redirect instead of routing through the codebase's centralized safeRedirectPath/router pattern. The rest of the app funnels auth redirects through safeRedirectPath (web/src/lib/redirect.ts:12, used at web/src/router.tsx:40,59,90 and web/src/pages/LoginPage.tsx:36), whose header comment explicitly states "both go through this so a caller-controlled value can never send the user to an external origin." The new 401 p…

🪰 Gadfly · advisory

🟡 **401 redirect hand-rolls encodeURIComponent + window.location.assign instead of reusing safeRedirectPath for the value construction used elsewhere for auth redirects** _maintainability · flagged by 1 model_ - **`web/src/lib/agent.ts:168-169` — 401 redirect hand-rolls the login redirect instead of routing through the codebase's centralized `safeRedirectPath`/router pattern.** The rest of the app funnels auth redirects through `safeRedirectPath` (`web/src/lib/redirect.ts:12`, used at `web/src/router.tsx:40,59,90` and `web/src/pages/LoginPage.tsx:36`), whose header comment explicitly states "both go through this so a caller-controlled value can never send the user to an external origin." The new 401 p… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Minor issues · 6 findings (1 with multi-model agreement)

Finding Where Models Lens
🟡 Journal date filter inputs hand-roll styling instead of reusing shared fieldControlClass, dropping text-base (iOS zoom-on-focus) and diverging focus-ring styling from every other input in the app web/src/editor/JournalPanel.tsx:89 2/5 maintainability
5 single-model findings (lower confidence)
Finding Where Model Lens
🟠 Same mobile-viewport 100vh bug as GardenEditorPage left unfixed — inconsistent with the PR's stated fix web/src/pages/PublicGardenPage.tsx:56 opencode/glm-5.2:cloud maintainability
🟡 Error toasts no longer auto-dismiss and the store has no cap, so repeated errors grow the toast stack/DOM unbounded web/src/components/ui/toast.tsx:40 glm-5.2:cloud performance
🟡 Persistent error toasts can stack unboundedly with no scroll/cap, pushing older ones off-screen and unreachable web/src/components/ui/toast.tsx:72 claude-code/sonnet error-handling
🟡 401 redirect hand-rolls encodeURIComponent + window.location.assign instead of reusing safeRedirectPath for the value construction used elsewhere for auth redirects web/src/lib/agent.ts:168 glm-5.2:cloud maintainability
filter built from three stacked ternary-spread branches; a compact helper or plain assignments would scale better as filter fields grow web/src/editor/JournalPanel.tsx:49 glm-5.2:cloud maintainability
Per-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 (authn/authz, injection, SSRF, unsafe deserialization, secret leakage, input validation, insecure defaults):

  • web/src/lib/agent.ts:163-170 — new 401 handling redirects to `/login?redirect=${back}` where back is encodeURIComponent(location.pathname + location.search). Verified this can't produce an open redirect: the source value is always the current same-origin path (never attacker-controlled input), it's percent-encoded before insertion, and the receiving LoginPage (web/src/pages/LoginPage.tsx:36) still passes it through the existing safeRedirectPath() allowlist (web/src/lib/redirect.ts:12), which rejects anything not starting with a single /. Belt-and-suspenders, no gap.
  • web/src/components/ui/toast.tsxitem.message is rendered via plain JSX interpolation ({item.message}), not dangerouslySetInnerHTML; React escapes it. New dismiss button is inert (onClick only). No XSS surface added.
  • web/src/editor/JournalPanel.tsx — new from/to state comes from <input type="date"> and flows into useJournal(gardenId, filter)api.get(..., { params: filter }), which builds the query string via URLSearchParams (web/src/lib/api.ts:47-51), not string concatenation — no query injection. Values are also plain YYYY-MM-DD, filtered client-side against min/max on the sibling input, and the diff description confirms no backend change (existing filter already accepted from/to).
  • web/src/pages/GardenEditorPage.tsx — CSS unit change only (100vh100dvh), no security surface.

Nothing to flag in this lane; the diff is clean from a security standpoint.

🎯 Correctness — No material issues found

useState is already imported. Everything checks out — imports resolve, param names match end to end (frontend from/to → Go handler c.Query("from"/"to") → service → store), date format is consistent (YYYY-MM-DD throughout, no timestamp/date lexicographic mismatch), the 401 redirect path format matches what safeRedirectPath expects, and the 401 status matches what the auth middleware actually returns. No logic bugs found in the toast, journal filter, chat 401 handling, or dvh changes.

Verdict: No material issues found

I verified end-to-end through actual source, not just the diff:

  • Journal date filter (web/src/editor/JournalPanel.tsx, internal/api/journal.go, internal/store/journal.go): confirmed from/to query param names match on both sides, and that observed_at is stored as a plain 'YYYY-MM-DD' string (internal/domain/domain.go:183), matching the <input type="date"> value format — so the store's >=/<= string comparisons (internal/store/journal.go:94,98) are safe lexicographic date comparisons, not a date-vs-timestamp mismatch that would silently exclude same-day entries.
  • 401-during-chat handling (web/src/lib/agent.ts:163-171): confirmed the backend's session middleware uniformly returns 401 for missing/expired sessions (internal/api/auth.go:126, multiple _test.go files), so branching on res.status === 401 is correct. Confirmed the redirect path built from location.pathname + location.search matches what safeRedirectPath (web/src/lib/redirect.ts) expects (must start with /), and that the redirect query-param name matches what LoginPage's validateSearch reads (web/src/router.tsx:74-90). The 401 branch returns before the generic !res.ok branch, so no double-handling.
  • Toast persistence (web/src/components/ui/toast.tsx): isError gate on the auto-dismiss effect is correct and doesn't reintroduce the timeout for error toasts; dismiss button wired to the store's actual dismiss action.
  • 100vh100dvh (web/src/pages/GardenEditorPage.tsx:405): straightforward, correct unit substitution; no other logic depends on the old value.

No cross-file breakage, no incorrect constants/formats, no logic bugs within this lens.

🧹 Code cleanliness & maintainability — Minor issues

Confirmed: fieldControlClass is the established shared pattern, used directly and via cn(fieldControlClass, ...) compact variants (e.g. ShareGardenModal.tsx:112), and text-base is documented in field.ts:13-14 as deliberately preventing iOS zoom-on-focus. The new From/To inputs in JournalPanel.tsx:84-90 and 94-100 bypass this entirely, inherit text-xs from the parent wrapper (JournalPanel.tsx:81), and use a divergent focus ring (focus-visible:ring-accent/40 vs. the standard focus:border-accent focus:ring-2 focus:ring-accent/30). The finding holds up under independent verification.

VERDICT: Minor issues

  • web/src/editor/JournalPanel.tsx:84-100 — The new From/To date filter inputs hand-roll their own styling (rounded-md border border-border bg-surface px-1.5 py-1 text-fg outline-none focus-visible:ring-2 focus-visible:ring-accent/40) instead of reusing the shared fieldControlClass (web/src/components/ui/field.ts:15-18). Every other text/date input in the app goes through it — directly via TextField (used a few lines below in this same file's Composer for the "Observed" date, and in PlopInspector.tsx:168 for "Planted"), or via cn(fieldControlClass, '…') for compact inline variants, e.g. ShareGardenModal.tsx:112 (cn(fieldControlClass, 'w-auto px-2 py-1 text-sm')) — exactly the pattern this filter bar needed and skipped. Concretely this drops text-base, which field.ts:13-14 documents as deliberate ("keeps iOS Safari from zooming on focus"); these inputs instead inherit text-xs from the parent wrapper div (JournalPanel.tsx:81), so focusing From/To on an iOS device triggers the zoom-on-focus behavior the shared abstraction exists to prevent — notable given the PR's other changes are explicitly mobile-polish fixes (100dvh). The focus ring also drifts (focus-visible:ring-accent/40 vs. the standard focus:border-accent focus:ring-2 focus:ring-accent/30), so these two inputs look and behave slightly differently from otherwise-identical date fields elsewhere in the editor.
    • Fix: className={cn(fieldControlClass, 'w-auto px-1.5 py-1 text-xs')} (or similar), matching the ShareGardenModal compact-variant pattern.

Everything else in the diff (toast dismiss button, 401 handling in agent.ts, the 100dvh swap) is small, focused, and consistent with the surrounding code's style and comment conventions — no other cleanliness concerns found.

Performance — No material issues found

No automatic mutation retries configured (only a query has retry: false explicitly for a different case). Toast growth is bounded by actual user-triggered actions, not automatic retries — not a real regression.

This is a small, low-risk diff (CSS unit swap, an early-return branch, a toast dismiss button, and a client-side filter object). I checked the two files most likely to hide a performance issue — journal.ts/JournalPanel.tsx (query-key/refetch behavior) and toast.tsx (list growth, mutation retry paths) — and found nothing that regresses efficiency.

VERDICT: No material issues found

  • The new filter object in JournalPanel.tsx:49 is recreated every render, but it only feeds a React Query queryKey (journal.ts:48), which hashes structurally — this doesn't cause extra fetches beyond actual from/to/objectId changes, and matches the pre-existing pattern for scopeObjectId.
  • Making error toasts persist until dismissed (toast.tsx:36-43) could in theory let the toast list grow, but pushes only happen from user-triggered mutation onError callbacks (verified via grep in web/src/lib/objects.ts), and no mutation retry is configured in the query client (web/src/lib/queryClient.ts), so growth is bounded by actual user actions, not an automatic loop.
  • The 100vh100dvh swap (GardenEditorPage.tsx:405) and the 401 early-return in streamChat (agent.ts:163-171) are simple, non-looping changes with no efficiency impact.
🧯 Error handling & edge cases — Minor issues

Confirmed as described — no issues there. The draft's core finding checks out against the actual code.

Corrected review

VERDICT: Minor issues

  • web/src/components/ui/toast.tsx:22,72 — Error toasts no longer auto-dismiss (by design, per the PR), but the toast store has no cap and Toaster's container (fixed bottom-4 ... flex flex-col items-center gap-2) has no max-height/overflow-y-auto. push (line 22) unconditionally appends with no size limit, and the container div (line 72) has no scroll/clamp styling. Before this change, a burst of failed mutations self-healed after 4s; now a run of failures (e.g. several rapid drag-and-drop edits each hitting ErrVersionConflict, or a flaky connection during a multi-step operation) leaves an ever-growing stack that pushes earlier toasts off the top of the viewport with no scrollbar to reach them — their only dismiss affordance (the ✕ button) becomes unreachable. Suggest capping the stack (e.g. drop/collapse beyond N) or adding max-h-* overflow-y-auto to the container now that toasts can accumulate indefinitely.

Everything else in this lens checked out:

  • agent.ts 401 branch: correctly placed before the generic !res.ok fallback, doesn't attempt to read a body it doesn't need, and handlers.onError fires before the synchronous window.location.assign, so the message is set before navigation — no ordering bug.
  • JournalPanel.tsx from/to inputs: min/max cross-constrain the two pickers so an inverted range can't be produced through the UI; empty-string filtering (from ? {...} : {}) matches JournalFilter's undefined-means-unfiltered contract.
  • GardenEditorPage.tsx 100vh100dvh is a pure CSS unit change, no logic/edge-case surface.
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

Security lens review of this bundle:

  • 401 → /login?redirect= (agent.ts:163-170): I traced the redirect handling. The back value is built from location.pathname + location.search — both are browser-managed, same-origin values; pathname cannot carry a protocol or host, so an attacker cannot inject an external origin here. The consumer (LoginPage.tsx:36) runs the redirect param through safeRedirectPath (redirect.ts:12), which rejects anything not starting with a single / (blocking //evil.com and https://…). Open-redirect is closed at the consumption point. Verified by reading web/src/lib/redirect.ts, web/src/pages/LoginPage.tsx, and the router guards in web/src/router.tsx. No issue.

  • Journal date filter (JournalPanel.tsx): from/to are <input type="date"> strings flowing into JournalFilterapi.get query params, which are serialized via URLSearchParams in buildUrl (api.ts:47-57). No path/query injection vector; the values are encoded and only ever appended as query params. Client-side min/max are UX constraints, not security boundaries. No backend change. Verified by reading web/src/lib/journal.ts and web/src/lib/api.ts.

  • Toast persistence / close button (toast.tsx) and 100dvh (GardenEditorPage.tsx): purely presentation/layout; no untrusted data handling, no authn/authz surface.

No security-relevant problems in this change.

🎯 Correctness — No material issues found

Verdict: No material issues found

I verified the four changes through the correctness lens:

  • Journal date filter (JournalPanel.tsx) — HTML date inputs emit YYYY-MM-DD, which is exactly the format observed_at is stored in (0008_journal.sql:22) and what the store compares against (internal/store/journal.go:93-100, observed_at >= ? / observed_at <= ?). String comparison is valid for this fixed-width format, and the range is inclusive on both ends as the UI implies. Empty strings are spread-conditionally omitted, so an unset bound doesn't filter — matching JournalFilter's "undefined = don't filter" contract (web/src/lib/journal.ts:32-38). The min/max cross-constraints on the inputs only bound the picker; even if a user bypasses them, the backend validDatePtr check (internal/service/journal.go:58) still validates each value, and an inverted range simply yields an empty result rather than an error. Correct.

  • Session-expiry 401 (agent.ts) — The new res.status === 401 branch sits after the abort check and before the generic !res.ok fallthrough, so it intercepts 401s first. location.pathname + location.search + encodeURIComponent is the right redirect-preserving construction. Matches the app's existing 401 treatment. Correct.

  • Error toast persistence (toast.tsx) — The effect early-returns for isError (no timer set) and otherwise keeps the 4s timer for info toasts; isError is correctly added to the dependency array. The dismiss button calls dismiss(item.id) for both tones. No logic bug.

  • 100dvh (GardenEditorPage.tsx) — A unit/viewport-correctness swap; calc(100dvh-8rem) uses the same arbitrary-value syntax as the prior 100vh form, which already compiled/applied. dvh reflects the dynamic viewport, which is the intended fix. Correct.

No semantic, formula, or boundary issue surfaced.

🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues found

  • web/src/lib/agent.ts:168-169 — 401 redirect hand-rolls the login redirect instead of routing through the codebase's centralized safeRedirectPath/router pattern. The rest of the app funnels auth redirects through safeRedirectPath (web/src/lib/redirect.ts:12, used at web/src/router.tsx:40,59,90 and web/src/pages/LoginPage.tsx:36), whose header comment explicitly states "both go through this so a caller-controlled value can never send the user to an external origin." The new 401 path in streamChat instead builds the URL by hand (encodeURIComponent(location.pathname + location.search) + window.location.assign('/login?redirect=…')). The value is derived from location, so it is always same-origin and not a security hole; the issue is duplication of a deliberately centralized pattern. Note the streamChat function legitimately can't use TanStack Router's redirect() (it's not in a loader/beforeLoad and uses a raw fetch), so the realistic fix is just to reuse safeRedirectPath for the value construction rather than the router throw. Small, not blocking.

  • web/src/editor/JournalPanel.tsx:89,99 — date-input control styling duplicated verbatim with no shared class constant. The rounded-md border border-border bg-surface px-1.5 py-1 text-fg outline-none focus-visible:ring-2 focus-visible:ring-accent/40 string is repeated identically for the From and To inputs (lines 89 and 99), and a near-identical variant already exists for the scope <select> at line 69 and the Clear button at line 109. A small const inputCls = '…' at module top would keep them from drifting. Trivial cleanliness nit.

  • web/src/editor/JournalPanel.tsx:49-53filter built from three stacked ternary-spread branches. The ...(scopeObjectId != null ? { objectId: scopeObjectId } : {}) idiom, stacked three times, reads as clever rather than clear. A compact/Object.fromEntries(Object.entries({...}).filter(...)) helper or plain assignments into a const filter: JournalFilter = {} would scale better as more filter fields appear. Trivial maintainability nit.

All three are cleanliness/maintainability nits, not blockers. The toast (error toasts don't auto-dismiss, dismiss button) and 100dvh changes are clean and follow the surrounding patterns.

Performance — Minor issues

Confirmed: push at line 22 has no cap, error toasts skip the setTimeout at line 40, and Toaster renders the whole array at line 73. The unbounded-growth finding survives. The second bullet is a self-described non-issue and was not in the findings JSON, so it's dropped.

VERDICT: Minor issues

  • web/src/components/ui/toast.tsx:40 — error toasts now accumulate with no bound. Previously every toast auto-dismissed at 4s, so the toasts array in the zustand store self-drained. Now error toasts never time out and are removed only by an explicit dismiss click. The store has no max-cap — push just appends (line 22: set((s) => ({ toasts: [...s.toasts, { id: nextId++, message, tone }] }))) — and Toaster renders the entire array (lines 73-75). Any path that fires repeated toast.error calls — a user mashing retry on a failing mutation, a background refetch storm that surfaces errors, or a stuck loop — will grow the stack and the DOM indefinitely, where before the 4s timeout provided natural GC. Suggested fix: cap the toasts array in push (e.g. drop the oldest when exceeding ~5), or at minimum drop the oldest error when an error is pushed while already at the cap — preserves "errors persist" without unbounded growth.

The 100dvh and streamChat 401-redirect changes are one-shot CSS/navigation changes with no performance surface.

🧯 Error handling & edge cases — No material issues found

I verified the key suspicion and it's clean. My initial concern was that the to date filter would be exclusive of the "To" day (a <= 'YYYY-MM-DD' against a timestamp would drop same-day entries). But the migration at internal/store/migrations/0008_journal.sql:22 shows observed_at is stored as 'YYYY-MM-DD' (date-only text), and internal/store/journal.go:97-99 does j.observed_at <= ? against that same date string — so to: 2025-05-15 correctly includes all May 15 entries. No boundary bug.

Verdict: No material issues found

  • Toast close-button / no-auto-dismiss for errors (web/src/components/ui/toast.tsx): verified the full component. The useEffect early-returns for errors (no timer leak), the close button calls dismiss(item.id), and the isError dep keeps the effect correct if tone ever changed. Clean.
  • Journal date filter (web/src/editor/JournalPanel.tsx): empty strings correctly don't filter (...(from ? {from} : {})), Clear resets both, and the min/max cross-constraints use || undefined so an empty opposite bound doesn't constrain. The to-inclusive-of-the-day concern was disproven by reading the schema and store query above. Clean.
  • 401 mid-chat (web/src/lib/agent.ts:163-171): the branch returns after handling, so it doesn't fall through into the !res.ok block. encodeURIComponent guards the redirect param. One minor, non-material observation: onError pushes a toast immediately before window.location.assign navigates away, so that toast is likely unmounted before it's seen — but the login page itself is the real signal, so this isn't a broken error path. Not worth flagging as a finding.
  • 100dvh change: pure CSS value swap, no error-handling surface.
kimi-k2.6:cloud (ollama-cloud) — No material issues found

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

🔒 Security — No material issues found

No material issues found

I reviewed all four changes through the security lens and found no authn/authz gaps, injection vectors, unsafe redirects, or secret leakage.

  • toast.tsx — purely a UI change (manual dismiss, conditional auto-dismiss). No security impact.
  • JournalPanel.tsx / journal.ts — date strings from native <input type="date"> are passed through the existing api.get() wrapper, which feeds them into URLSearchParams. Properly encoded, no injection path.
  • agent.ts 401 redirectlocation.pathname + location.search is same-origin by construction, encodeURIComponent places it safely into the query string, and the destination is validated on the login side by safeRedirectPath (rejects // protocol-relative and non-/ prefixes). Verified against redirect.ts:12 and LoginPage.tsx:36. No open-redirect vulnerability.
  • GardenEditorPage.tsx — CSS viewport unit swap, zero security relevance.
🎯 Correctness — No material issues found

No material issues found

Through the Correctness lens, the four changes are all well-reasoned and free of logic bugs:

  • 100dvh — The dynamic viewport unit correctly fixes the mobile Safari/Chrome overflow issue described; 100vh on mobile browsers includes the space behind the URL bar, while 100dvh tracks the actual visible viewport.
  • Toast persistence — The isError guard correctly skips setTimeout only for error toasts, preserving existing behavior for info toasts. The useEffect dependency list includes isError, so toggling a toast's tone would correctly reset/clear timers.
  • Journal date filter — Verified that JournalFilter in web/src/lib/journal.ts:36-37 already declares from?: string and to?: string, and useJournal spreads the filter into API params. The UI inputs use HTML min/max to prevent invalid ranges, and empty strings are correctly filtered out before being passed to the query.
  • 401 handling in chat — Checked that the rest of the app (router requireAuth at web/src/router.tsx:40, meQueryOptions at web/src/lib/auth.ts:35) treats 401 as unauthenticated and redirects to /login with a redirect search param. The new agent code follows the same convention, using encodeURIComponent on location.pathname + location.search, which matches the router's pattern.

No semantic errors, miscalculations, or boundary-condition mishandling were found.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

Reviewed through the code cleanliness & maintainability lens. The changes are concise, well-commented, and consistent with surrounding code:

  • toast.tsx — Extracting isError avoids repeating item.tone === 'error' three times; the close button follows the same styling idiom (focus-visible:ring-2 focus-visible:ring-accent/40) used elsewhere.
  • JournalPanel.tsx — The date inputs reuse the existing inline styling pattern (border-border bg-surface … focus-visible:ring-accent/40) already present in the sibling <select> and other editor components. Using raw <input> instead of the project's TextField component is correct here because TextField renders its own <label> wrapper, whereas these inputs need tiny inline "From"/"To" labels.
  • agent.ts — The 401 check is placed correctly between the network-failure path and the general !res.ok path, keeping the error-handling flow linear and readable.
  • GardenEditorPage.tsx — One-line CSS change with a clear explanatory comment.

Nothing to flag for duplication, naming, structure, or pattern drift.

Performance — No material issues found

Verdict: No material issues found

I examined all four changes through the performance lens and found nothing material.

  • Mobile viewport (100dvh) — CSS unit swap. dvh updates as browser chrome shows/hides; this is expected layout work and no regression over 100vh.
  • Session expiry mid-chat — Single synchronous path: fire onError callback then navigate. No loop, no allocation hotspot, no blocking I/O beyond the single redirect.
  • Error toasts persist — The toast store uses small objects and array clones on push/dismiss. Error toasts can now accumulate without auto-dismiss, which is unbounded in principle, but bounded by user action in practice and the memory footprint per toast is negligible. This is a deliberate UX tradeoff, not an efficiency regression.
  • Journal date filterfrom/to are <input type="date"> state changes (not keystroke-streaming), so each picker selection yields exactly one state update → one React Query key change → one fetch. The filter object is reconstructed every render, but React Query serializes query keys structurally, so identical values do not trigger spurious refetches. The pages.flatMap((p) => p.entries) on every render is O(total loaded entries) and only used for rendering, with no downstream hook-dependency cascade.

All clean from a performance perspective.

🧯 Error handling & edge cases — No material issues found

No material issues found

  • toast.tsx: The early-return for isError in the useEffect correctly prevents auto-dismiss of error toasts, while info toasts still time out. No swallowed cleanup — the existing clearTimeout(t) teardown is preserved. The isError dependency is stable and correct.
  • JournalPanel.tsx: Empty-string initialization for from/to is safely handled by conditional spreading (...(from ? { from } : {})). The Clear button resets both states. Browser-native min/max constraints on the date inputs guard against invalid ranges. No nil-dereference issues in the filter construction or entries fallback.
  • agent.ts: The 401 check precedes the generic !res.ok handler, so the correct error path is taken. The signal?.aborted check in the fetch catch block already filters out user-initiated aborts before the 401 logic is reached. window.location.assign is used for the hard redirect, matching the rest of the app's 401 treatment. No unhandled edge cases in the session-expiry flow.
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

I traced every tainted input path in this diff through to its sink and verified each is either constrained or already protected by an existing layer:

  • Journal date filter (web/src/editor/JournalPanel.tsx:47-53): from/to come from <input type="date"> and flow to JournalFilter → query params → service.JournalQuery.From/To. I verified at internal/service/journal.go:58 that validDatePtr (defined at internal/service/plantings.go:309) parses each value with time.Parse(dateLayout, …) and rejects anything that isn't YYYY-MM-DD with ErrInvalidInput (→ 400). The store layer (internal/store/journal.go:93-99) binds them as parameterized SQL args (args = append(args, *f.From)), not string interpolation, so no SQL injection even for a value that slipped past. The malformed-date 400 path is covered by TestJournalFiltersAPI at internal/api/journal_test.go:131-134. No XSS: the values render only as the input's own value and as React-escaped list text.

  • Agent 401 redirect (web/src/lib/agent.ts:168-169): back = encodeURIComponent(location.pathname + location.search) is the current page's own path/query (inherently same-origin) and is only ever consumed by /login, which I verified at web/src/pages/LoginPage.tsx:36 routes it through safeRedirectPath (web/src/lib/redirect.ts:12-13) — that function rejects anything not starting with a single / and explicitly blocks protocol-relative //evil.com. No open-redirect surface; the redirect target is both attacker-uncontrollable (it's the page you're already on) and validated.

  • Toast (web/src/components/ui/toast.tsx): item.message is rendered as React text children (<span className="flex-1">{item.message}</span>), so it's auto-escaped — no HTML injection from error messages. No auth/input concerns.

  • 100dvh (GardenEditorPage): CSS-only, no security surface.

Nothing in the security lens is materially wrong.

🎯 Correctness — No material issues found

I'll verify each change against the actual checked-out code through the correctness lens.
Now let me verify how the backend interprets from/to — the UI sends YYYY-MM-DD strings, and I need to confirm the backend treats them as expected (inclusive date bounds, not strict less-than).
Now I want to verify the streamChat 401 path more carefully — specifically whether handlers.onError triggers any UI that races with the window.location.assign. Let me check how the chat UI uses onError.
Now let me verify the JournalPanel — check that the date filter is actually wired correctly with the useJournal hook, and confirm the from/to are sent as query params properly. Let me also check the location global usage in agent.ts.
Now I have enough to assess correctness. Let me trace through the key concerns:

1. Toast — error persistence: The useEffect returns early when isError (no timer set). Close button calls dismiss. Logic is sound. The dependency array includes isError which is fine.

2. Agent 401 handling: On 401, calls handlers.onError then window.location.assign. The onError sets error state and triggers refresh.everything() (which invalidates queries). The navigation will happen, so the React state update is moot but harmless. The redirect uses location.pathname + location.search encoded. This matches the router's ?redirect= convention. One concern: location is referenced bare (line 168) — in browser this is window.location, works fine. The redirect param is read back by LoginPage via safeRedirectPath which validates same-origin path. Good.

3. Journal date filter: Sends from/to as YYYY-MM-DD strings. Backend store/journal.go:93-99 does observed_at >= ? and observed_at <= ? with string comparison. Since observed_at is stored as YYYY-MM-DD (per migration 0008), lexicographic string comparison of YYYY-MM-DD is correct chronological comparison. Inclusive on both ends. UI max/min constraints prevent from > to. Correct.

4. 100dvh: CSS change, no logic implications for correctness lens.

Let me double check one thing — whether the from/to empty-string handling in buildUrl (api.ts line 52) properly omits them. Yes, v === '' is skipped. And the spread ...(from ? { from } : {}) means empty string is falsy so from key is omitted entirely. Consistent.

Let me verify the backend validDatePtr to ensure the format matches what the UI sends.
I've verified all four changes through the correctness lens. My findings:

VERDICT: No material issues found

  • Toast persistence (toast.tsx): The useEffect early-returns when isError, so no timer is set — errors stay until the ✕ button calls dismiss. isError is derived from the immutable item.tone, so it never flips for a given item; the dep-array addition is harmless. Verified by reading the full file.
  • Agent 401 (agent.ts:163-171): Fires onError then window.location.assign to /login?redirect=<encoded path>. The encoded location.pathname + location.search matches the ?redirect= search param the router (router.tsx:84-86) and LoginPage (LoginPage.tsx:36 via safeRedirectPath) expect; safeRedirectPath rejects //evil.com / absolute URLs, and a same-origin path always starts with /, so the value is safe. The onErrorrefresh.everything() invalidations racing with navigation are wasted work, not a correctness bug. Verified ApiError.isUnauthorized exists at api.ts:30 for parity with the rest of the app.
  • Journal date filter (JournalPanel.tsx): UI emits YYYY-MM-DD strings (HTML date input format). Backend validates with time.Parse("2006-01-02", …) (plantings.go:309-314) and stores observed_at as YYYY-MM-DD (migrations/0008_journal.sql:22), compared as >=/<= string bounds (store/journal.go:93-99) — lexicographic order on YYYY-MM-DD IS chronological, inclusive on both ends. The max={to || undefined} / min={from || undefined} constraints prevent an inverted range in the picker. Empty strings are falsy so the spread omits the key, and buildUrl (api.ts:52) also skips '' — double-guarded. The inline filter object is rebuilt each render, but that was already true before this change and TanStack Query does structural key comparison. Verified end-to-end.
  • 100dvh (GardenEditorPage.tsx): Pure CSS substitution, 100dvh is the correct dynamic-viewport unit for the stated mobile-Safari/Chrome problem; no logic impact.

No semantic, formula, unit, or boundary issues found in this lens.

🧹 Code cleanliness & maintainability — Minor issues

Confirmed: PublicGardenPage.tsx:56 still uses calc(100vh-8rem) and is the only remaining call site of that pattern, while the PR fixed the equivalent layout in GardenEditorPage to 100dvh. The finding survives.

The other two draft findings were self-described as non-issues ("this is fine", "a consistency/maintainability nit") and weren't carried as findings in the JSON anyway. Only the PublicGardenPage inconsistency survives.

VERDICT: Minor issues

  • web/src/pages/PublicGardenPage.tsx:56 — The 100dvh fix is applied only to the authenticated editor. PublicGardenPage.tsx:56 still uses calc(100vh-8rem) for the same full-height layout on the same class of mobile browsers — an unauthenticated visitor on mobile Safari hits exactly the bug the PR says it's fixing (canvas bottom / Fit button pushed under browser chrome when the URL bar is visible). Verified by grep: this is the only remaining calc(100…-8rem) call site, and the PR only touched GardenEditorPage. Either the fix is wrong for one of the two pages, or PublicGardenPage was missed.
Performance — No material issues found

Through the Performance lens, I examined each change for refetch storms, unbounded growth, allocations on hot paths, and unnecessary re-renders. Verifying against the checked-out code:

  • Journal filter object (JournalPanel.tsx:49-53): A new filter object is built each render and feeds useJournal's queryKey. I checked journal.ts:42-58 — the key is ['gardens', gardenId, 'journal', filter]. TanStack Query hashes keys structurally (hashKey does deep comparison), so an equal-valued new object does not trigger a refetch. This was already the pattern before the PR (the old line created { objectId } / {} inline too), so no regression. Date inputs fire onChange once per complete date selection, not per keystroke, so each from/to change is one refetch — no storm, no debounce needed.

  • Error toast persistence (toast.tsx:40-43): Errors no longer auto-dismiss, so they accumulate until dismissed. I checked there's no automatic retry loop that would pump errors in the background (mutations don't configure retry, and streamChat doesn't loop). Accumulation is bounded by user-initiated actions and each toast is individually dismissible, so this is not an unbounded-growth concern.

  • streamChat 401 branch (agent.ts:163-171): window.location.assign navigates away — no path that does repeated work.

  • 100dvh and toast layout changes: pure CSS/render, no perf impact.

No material issues found in my lane.

🧯 Error handling & edge cases — No material issues found

I'll examine the changed files and their surrounding context through the error-handling/edge-case lens.
Now let me check how the rest of the app handles 401 redirects and the ApiError.unauthorized pattern to compare with the agent.ts change.
Let me check the streamChat callers and whether the 401 navigation interacts with any abort/cleanup logic.
Now let me check how the login page reads the redirect param, to verify the agent.ts redirect is consistent and won't be lost.
Now I have full context. Let me think through the edge cases for each change.

Toast change: Error toasts no longer auto-dismiss; only info ones time out. The useEffect early-returns for errors. The dep array includes isError (stable per item). One concern: an error toast that never auto-dismisses — if a user walks away with many failed mutations, toasts accumulate. But there's now a ✕ close button, so dismissible. This is intentional per the PR. No swallowed error. Fine.

One subtle issue: the close button is rendered for both info and error toasts. For an info toast that's about to auto-dismiss at 4s, the button is harmless. Fine.

Journal date filter: from/to are raw strings from <input type="date">. Browsers emit YYYY-MM-DD or empty string. Empty string is falsy → omitted from filter. The max={to || undefined} / min={from || undefined} constraints prevent inverted ranges in the UI. But there's no validation that from <= to if a user manually... actually with max/min constraints the native picker enforces it. Edge case: if to is set and user clears to via Clear button, from's max becomes undefined — fine.

One real concern: the filter object is reconstructed every render as a new object literal. useJournal uses it as a query key (via journalKey['gardens', gardenId, 'journal', filter]). TanStack Query serializes the key; a new object with same contents hashes equal, so no infinite refetch. But api.get spreads ...filter into params — { from: '2026-01-01', to: undefined }? No — empty strings are omitted via the spread ...(from ? {from} : {}), so undefined never appears. The buildUrl also skips ''. Good. No issue.

Agent 401 handling: On 401, calls onError then window.location.assign('/login?redirect=...'). The onError sets setPending(null) and setError(message) in ChatPanel, then refresh.everything(). But immediately after, window.location.assign navigates away — the whole page unloads. So the setError/setPending state updates are moot (page is going away). Not a bug, just dead work.

A more material concern: onError is called before the navigation, and ChatPanel.onError calls refresh.everything() which fires API queries — those will race with the navigation. But navigation unloads everything; the in-flight queries get cancelled by the browser. Not a real problem.

Is there an issue that window.location.assign happens during render cycle / async fetch resolution? It's in an async function after await fetch, so it's in a microtask — fine, navigation is allowed.

The redirect uses location.pathname + location.search — but the chat is on the garden editor page; hash is dropped. Minor, not material. The encodeURIComponent is correct.

One thing: this bypasses safeRedirectPath validation on the writing side (the agent.ts constructs the redirect), but the reading side (LoginPage) runs it through safeRedirectPath, so it's sanitized on consumption. Since location.pathname always starts with / and is same-origin, it's safe. Fine.

100dvh change: Pure CSS, no error path. dvh has good support. Fallback: browsers that don't support dvh will ignore the declaration... actually no, if calc(100dvh-8rem) is invalid in an old browser, the whole h-[...] class is invalid and height falls back to auto/content. That could collapse the editor layout on very old browsers. But Tailwind arbitrary values — h-[calc(100dvh-8rem)] — produce height: calc(100dvh-8rem). Old browsers ignoring dvh → invalid → height unset. This is a potential regression for old browser support, but it's a deliberate tradeoff per the PR. Not an error-handling issue per se. Outside my lens really.

Let me check if there's any issue with the date filter when the journal query errors (e.g., 400 from malformed date). The browser <input type="date"> always emits valid YYYY-MM-DD or empty, so malformed dates can't be sent from this UI. Good — the existing 400 test for ?from=nonsense won't be triggered from this UI.

I've found nothing material in my lens. Let me double check one thing: the toast useEffect early return — when isError is true, the effect returns undefined (no cleanup). When the component re-renders with same isError, effect doesn't re-run (deps unchanged). Fine. When an info toast transitions... tone is fixed per toast (set at push), so isError never changes for a given item. Fine.

Verdict: No material issues found in the error-handling/edge-case lens. The changes are careful — empty-string falsiness is handled, the 401 path navigates correctly, error toasts are dismissible, and the existing 400-on-malformed-date backend validation is unreachable from this UI because native date inputs constrain the format.

Let me verify one last thing — that there's no path where a 401 during streaming (not initial fetch) needs handling. The 401 check is only on the initial response, before the read loop. Mid-stream 401 can't happen (response already 200). Fine.

No material issues found.VERDICT: No material issues found

I reviewed all four changed files through the error-handling/edge-case lens, verifying against the checked-out code (not the diff alone):

  • toast.tsx — Error toasts early-return from the timeout effect; isError is fixed per item (tone set at push), so it never flips and re-triggers the effect. The ✕ button works for both tones. Info toasts still time out at 4s. No swallowed dismissal, no accumulation-without-escape (close button exists). Verified by reading the full file.
  • JournalPanel.tsx date filter — Empty from/to are falsy and omitted via conditional spread, so they never reach the query as undefined/'' params. max={to || undefined}/min={from || undefined} prevent inverted ranges at the input level. The backend's ?from=nonsense → 400 path (verified in internal/api/journal_test.go:131) is unreachable from this UI because native <input type="date"> only emits YYYY-MM-DD or empty — never arbitrary strings. The reconstructed-each-render filter object is fine: TanStack serializes query keys by value, so no spurious refetch. Verified against lib/journal.ts (JournalFilter, useJournal, buildUrl's ''/null/undefined skip) and the store layer.
  • agent.ts 401 — Placed correctly: after the fetch try/catch, before the generic !res.ok branch, so a 401 is not swallowed into the "assistant unavailable" message. onError fires before window.location.assign, so ChatPanel's setPending(null)/refresh.everything() run but are moot (page unloads). The redirect target location.pathname + location.search is always same-origin and starts with /, and LoginPage runs it through safeRedirectPath on consumption (pages/LoginPage.tsx:36), so it's sanitized at read time. encodeURIComponent is applied. Verified against ChatPanel.tsx:84 and lib/redirect.ts.
  • GardenEditorPage.tsx 100dvh — Pure CSS substitution; no error path introduced. (Old-browser fallback for unsupported dvh units is a layout/compat concern, outside this lens.)

No unhappy paths are mishandled, no errors swallowed, no missing cleanup.

opencode/kimi-k2.6:cloud (opencode) — 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, I verified each change against the live code:

  • 401 redirect in chat (web/src/lib/agent.ts:167-169): The redirect target is encodeURIComponent(location.pathname + location.search), which is then consumed by the login page's safeRedirectPath (web/src/lib/redirect.ts:12-14). That helper rejects protocol-relative (//evil.com) and absolute URLs, so even a manipulated query string cannot turn this into an open redirect. The flow matches the existing ApiError.isUnauthorized/login?redirect= pattern used by the router guards.

  • Journal date filter (web/src/editor/JournalPanel.tsx:47-53): from/to strings pass through to api.get with parameterized query encoding (URLSearchParams). The backend (internal/api/journal.go:59-63) forwards them to the service, which validates them via validDatePtr (internal/service/plantings.go:309-315) against the strict 2006-01-02 layout. The store (internal/store/journal.go:93-99) binds them as ? placeholders, so SQL injection is not possible.

  • Toast persistence & close button (web/src/components/ui/toast.tsx): The message is rendered as a React text node inside a <span>, which is inherently escaped. No new injection surface is introduced.

Nothing in this diff creates or widens an authn/authz gap, injection vector, unsafe deserialization path, secret leak, or missing input-validation hole.

🎯 Correctness — No material issues found

Verdict: No material issues found

Through the correctness lens, I scrutinized each change for logic bugs, semantic errors, and incorrect behavior:

Mobile viewport (100dvh) — Verified GardenEditorPage.tsx:405. 100dvh correctly uses the dynamic viewport height, fixing the mobile Safari/Chrome issue where 100vh includes the URL bar-hidden area. This is the correct CSS fix for the described problem.

Session expiry in chat (401 handling) — Verified web/src/lib/agent.ts:163-171. The 401 check is placed correctly after the fetch response and before the SSE body read. I traced the redirect URL construction (encodeURIComponent(location.pathname + location.search)) through to LoginPage.tsx:36 and safeRedirectPath. TanStack Router decodes %2F back to / when parsing search params, so safeRedirectPath receives /gardens/... and accepts it. The behavior matches the existing router-guard 401 pattern exactly. No logic error.

Sticky error toasts — Verified web/src/components/ui/toast.tsx:36-43. The useEffect correctly skips the auto-dismiss timer when isError is true. Adding isError to the dependency array is correct because it's read inside the effect. The close button properly calls dismiss(item.id). No behavioral bug introduced.

Journal date filter — Verified web/src/editor/JournalPanel.tsx:47-114 and backend internal/store/journal.go:93-99 / internal/service/journal.go:54-85. The UI passes from/to strings directly to the existing JournalFilter/useJournal API. Empty strings are excluded from the filter via conditional spreads, and api.ts:52 strips empty-string params from the query string. The backend uses inclusive >= / <= on observed_at with YYYY-MM-DD string comparison, which is correct for ISO date ordering. HTML min/max attributes provide native browser validation. No backend change means no risk of store or service-layer regressions.

I also verified that tsc --noEmit passes per the PR description, and the code structure is consistent with existing patterns.

No material correctness issues were found.

🧹 Code cleanliness & maintainability⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run returned no output

Performance — No material issues found

No material issues found.

These changes are surface-level UX fixes (viewport unit, error message wording, toast lifetime, date inputs). None touch hot loops, query patterns, or allocation volume in a material way.

  • toast.tsx — Adding isError to the useEffect dep array is correct; it does not introduce extra effect runs because item.id and dismiss are stable, and the effect still bails early for errors. Persisting error toasts in the DOM is bounded by user-initiated actions, not background polling.
  • JournalPanel.tsx — The inline filter object is recreated every render, but the old code already used the same pattern (scopeObjectId); TanStack Query serializes keys, so this does not trigger spurious refetches. Date inputs commit values discretely, so state churn is low.
  • agent.ts — The new 401 branch is a single synchronous check before the existing stream handling. No I/O or allocation change.
  • GardenEditorPage.tsx100dvh vs 100vh is a CSS string change with no runtime work implications.
🧯 Error handling & edge cases — No material issues found

No material issues found.

  • web/src/lib/agent.ts: The 401 handling correctly intercepts the status before the generic !res.ok branch. The redirect URL construction (encodeURIComponent(location.pathname + location.search)) preserves the current path and query, matching the app's existing login-redirect convention (safeRedirectPath). window.location.assign after handlers.onError means the React state update from setError may still flush briefly before navigation, but this is harmless and consistent with how the rest of the app handles hard redirects.
  • web/src/components/ui/toast.tsx: Error toasts skip the timeout; the effect cleanup correctly clears any pending timer on unmount. The close button safely calls dismiss(item.id), which filters the store array.
  • web/src/editor/JournalPanel.tsx: Empty strings are correctly omitted from the filter object via spread conditionals. The min/max attributes on the date inputs are guarded with || undefined, which React correctly renders as absent. The type="date" input ensures only valid YYYY-MM-DD values reach state.
  • web/src/pages/GardenEditorPage.tsx: The 100dvh change is a CSS progressive enhancement for mobile browsers; no error-handling implications.

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** · 6 findings (1 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟡 | Journal date filter inputs hand-roll styling instead of reusing shared fieldControlClass, dropping text-base (iOS zoom-on-focus) and diverging focus-ring styling from every other input in the app | `web/src/editor/JournalPanel.tsx:89` | 2/5 | maintainability | <details><summary>5 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | Same mobile-viewport 100vh bug as GardenEditorPage left unfixed — inconsistent with the PR's stated fix | `web/src/pages/PublicGardenPage.tsx:56` | opencode/glm-5.2:cloud | maintainability | | 🟡 | Error toasts no longer auto-dismiss and the store has no cap, so repeated errors grow the toast stack/DOM unbounded | `web/src/components/ui/toast.tsx:40` | glm-5.2:cloud | performance | | 🟡 | Persistent error toasts can stack unboundedly with no scroll/cap, pushing older ones off-screen and unreachable | `web/src/components/ui/toast.tsx:72` | claude-code/sonnet | error-handling | | 🟡 | 401 redirect hand-rolls encodeURIComponent + window.location.assign instead of reusing safeRedirectPath for the value construction used elsewhere for auth redirects | `web/src/lib/agent.ts:168` | glm-5.2:cloud | maintainability | | ⚪ | filter built from three stacked ternary-spread branches; a compact helper or plain assignments would scale better as filter fields grow | `web/src/editor/JournalPanel.tsx:49` | 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 (authn/authz, injection, SSRF, unsafe deserialization, secret leakage, input validation, insecure defaults): - **`web/src/lib/agent.ts:163-170`** — new 401 handling redirects to `` `/login?redirect=${back}` `` where `back` is `encodeURIComponent(location.pathname + location.search)`. Verified this can't produce an open redirect: the source value is always the current same-origin path (never attacker-controlled input), it's percent-encoded before insertion, and the receiving `LoginPage` (`web/src/pages/LoginPage.tsx:36`) still passes it through the existing `safeRedirectPath()` allowlist (`web/src/lib/redirect.ts:12`), which rejects anything not starting with a single `/`. Belt-and-suspenders, no gap. - **`web/src/components/ui/toast.tsx`** — `item.message` is rendered via plain JSX interpolation (`{item.message}`), not `dangerouslySetInnerHTML`; React escapes it. New dismiss button is inert (`onClick` only). No XSS surface added. - **`web/src/editor/JournalPanel.tsx`** — new `from`/`to` state comes from `<input type="date">` and flows into `useJournal(gardenId, filter)` → `api.get(..., { params: filter })`, which builds the query string via `URLSearchParams` (`web/src/lib/api.ts:47-51`), not string concatenation — no query injection. Values are also plain `YYYY-MM-DD`, filtered client-side against `min`/`max` on the sibling input, and the diff description confirms no backend change (existing filter already accepted `from`/`to`). - **`web/src/pages/GardenEditorPage.tsx`** — CSS unit change only (`100vh`→`100dvh`), no security surface. Nothing to flag in this lane; the diff is clean from a security standpoint. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> `useState` is already imported. Everything checks out — imports resolve, param names match end to end (frontend `from`/`to` → Go handler `c.Query("from"/"to")` → service → store), date format is consistent (`YYYY-MM-DD` throughout, no timestamp/date lexicographic mismatch), the 401 redirect path format matches what `safeRedirectPath` expects, and the 401 status matches what the auth middleware actually returns. No logic bugs found in the toast, journal filter, chat 401 handling, or dvh changes. ## Verdict: No material issues found I verified end-to-end through actual source, not just the diff: - **Journal date filter** (`web/src/editor/JournalPanel.tsx`, `internal/api/journal.go`, `internal/store/journal.go`): confirmed `from`/`to` query param names match on both sides, and that `observed_at` is stored as a plain `'YYYY-MM-DD'` string (`internal/domain/domain.go:183`), matching the `<input type="date">` value format — so the store's `>=`/`<=` string comparisons (`internal/store/journal.go:94,98`) are safe lexicographic date comparisons, not a date-vs-timestamp mismatch that would silently exclude same-day entries. - **401-during-chat handling** (`web/src/lib/agent.ts:163-171`): confirmed the backend's session middleware uniformly returns `401` for missing/expired sessions (`internal/api/auth.go:126`, multiple `_test.go` files), so branching on `res.status === 401` is correct. Confirmed the redirect path built from `location.pathname + location.search` matches what `safeRedirectPath` (`web/src/lib/redirect.ts`) expects (must start with `/`), and that the `redirect` query-param name matches what `LoginPage`'s `validateSearch` reads (`web/src/router.tsx:74-90`). The 401 branch returns before the generic `!res.ok` branch, so no double-handling. - **Toast persistence** (`web/src/components/ui/toast.tsx`): `isError` gate on the auto-dismiss effect is correct and doesn't reintroduce the timeout for error toasts; dismiss button wired to the store's actual `dismiss` action. - **`100vh` → `100dvh`** (`web/src/pages/GardenEditorPage.tsx:405`): straightforward, correct unit substitution; no other logic depends on the old value. No cross-file breakage, no incorrect constants/formats, no logic bugs within this lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Confirmed: `fieldControlClass` is the established shared pattern, used directly and via `cn(fieldControlClass, ...)` compact variants (e.g. `ShareGardenModal.tsx:112`), and `text-base` is documented in `field.ts:13-14` as deliberately preventing iOS zoom-on-focus. The new From/To inputs in `JournalPanel.tsx:84-90` and `94-100` bypass this entirely, inherit `text-xs` from the parent wrapper (`JournalPanel.tsx:81`), and use a divergent focus ring (`focus-visible:ring-accent/40` vs. the standard `focus:border-accent focus:ring-2 focus:ring-accent/30`). The finding holds up under independent verification. ## VERDICT: Minor issues - **`web/src/editor/JournalPanel.tsx:84-100`** — The new From/To date filter inputs hand-roll their own styling (`rounded-md border border-border bg-surface px-1.5 py-1 text-fg outline-none focus-visible:ring-2 focus-visible:ring-accent/40`) instead of reusing the shared `fieldControlClass` (`web/src/components/ui/field.ts:15-18`). Every other text/date input in the app goes through it — directly via `TextField` (used a few lines below in this same file's `Composer` for the "Observed" date, and in `PlopInspector.tsx:168` for "Planted"), or via `cn(fieldControlClass, '…')` for compact inline variants, e.g. `ShareGardenModal.tsx:112` (`cn(fieldControlClass, 'w-auto px-2 py-1 text-sm')`) — exactly the pattern this filter bar needed and skipped. Concretely this drops `text-base`, which `field.ts:13-14` documents as deliberate ("keeps iOS Safari from zooming on focus"); these inputs instead inherit `text-xs` from the parent wrapper div (`JournalPanel.tsx:81`), so focusing From/To on an iOS device triggers the zoom-on-focus behavior the shared abstraction exists to prevent — notable given the PR's other changes are explicitly mobile-polish fixes (100dvh). The focus ring also drifts (`focus-visible:ring-accent/40` vs. the standard `focus:border-accent focus:ring-2 focus:ring-accent/30`), so these two inputs look and behave slightly differently from otherwise-identical date fields elsewhere in the editor. - Fix: `className={cn(fieldControlClass, 'w-auto px-1.5 py-1 text-xs')}` (or similar), matching the `ShareGardenModal` compact-variant pattern. Everything else in the diff (toast dismiss button, 401 handling in `agent.ts`, the `100dvh` swap) is small, focused, and consistent with the surrounding code's style and comment conventions — no other cleanliness concerns found. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No automatic mutation retries configured (only a query has `retry: false` explicitly for a different case). Toast growth is bounded by actual user-triggered actions, not automatic retries — not a real regression. This is a small, low-risk diff (CSS unit swap, an early-return branch, a toast dismiss button, and a client-side filter object). I checked the two files most likely to hide a performance issue — `journal.ts`/`JournalPanel.tsx` (query-key/refetch behavior) and `toast.tsx` (list growth, mutation retry paths) — and found nothing that regresses efficiency. VERDICT: No material issues found - The new `filter` object in `JournalPanel.tsx:49` is recreated every render, but it only feeds a React Query `queryKey` (`journal.ts:48`), which hashes structurally — this doesn't cause extra fetches beyond actual `from`/`to`/`objectId` changes, and matches the pre-existing pattern for `scopeObjectId`. - Making error toasts persist until dismissed (`toast.tsx:36-43`) could in theory let the toast list grow, but pushes only happen from user-triggered mutation `onError` callbacks (verified via `grep` in `web/src/lib/objects.ts`), and no mutation retry is configured in the query client (`web/src/lib/queryClient.ts`), so growth is bounded by actual user actions, not an automatic loop. - The `100vh` → `100dvh` swap (`GardenEditorPage.tsx:405`) and the 401 early-return in `streamChat` (`agent.ts:163-171`) are simple, non-looping changes with no efficiency impact. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Confirmed as described — no issues there. The draft's core finding checks out against the actual code. ## Corrected review **VERDICT: Minor issues** - `web/src/components/ui/toast.tsx:22,72` — Error toasts no longer auto-dismiss (by design, per the PR), but the toast store has no cap and `Toaster`'s container (`fixed bottom-4 ... flex flex-col items-center gap-2`) has no `max-height`/`overflow-y-auto`. `push` (line 22) unconditionally appends with no size limit, and the container div (line 72) has no scroll/clamp styling. Before this change, a burst of failed mutations self-healed after 4s; now a run of failures (e.g. several rapid drag-and-drop edits each hitting `ErrVersionConflict`, or a flaky connection during a multi-step operation) leaves an ever-growing stack that pushes earlier toasts off the top of the viewport with no scrollbar to reach them — their only dismiss affordance (the ✕ button) becomes unreachable. Suggest capping the stack (e.g. drop/collapse beyond N) or adding `max-h-* overflow-y-auto` to the container now that toasts can accumulate indefinitely. Everything else in this lens checked out: - `agent.ts` 401 branch: correctly placed before the generic `!res.ok` fallback, doesn't attempt to read a body it doesn't need, and `handlers.onError` fires before the synchronous `window.location.assign`, so the message is set before navigation — no ordering bug. - `JournalPanel.tsx` from/to inputs: `min`/`max` cross-constrain the two pickers so an inverted range can't be produced through the UI; empty-string filtering (`from ? {...} : {}`) matches `JournalFilter`'s `undefined`-means-unfiltered contract. - `GardenEditorPage.tsx` `100vh`→`100dvh` is a pure CSS unit change, no logic/edge-case surface. </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** Security lens review of this bundle: - **401 → `/login?redirect=` (agent.ts:163-170)**: I traced the redirect handling. The `back` value is built from `location.pathname + location.search` — both are browser-managed, same-origin values; `pathname` cannot carry a protocol or host, so an attacker cannot inject an external origin here. The consumer (`LoginPage.tsx:36`) runs the `redirect` param through `safeRedirectPath` (`redirect.ts:12`), which rejects anything not starting with a single `/` (blocking `//evil.com` and `https://…`). Open-redirect is closed at the consumption point. Verified by reading `web/src/lib/redirect.ts`, `web/src/pages/LoginPage.tsx`, and the router guards in `web/src/router.tsx`. No issue. - **Journal date filter (JournalPanel.tsx)**: `from`/`to` are `<input type="date">` strings flowing into `JournalFilter` → `api.get` query params, which are serialized via `URLSearchParams` in `buildUrl` (`api.ts:47-57`). No path/query injection vector; the values are encoded and only ever appended as query params. Client-side `min`/`max` are UX constraints, not security boundaries. No backend change. Verified by reading `web/src/lib/journal.ts` and `web/src/lib/api.ts`. - **Toast persistence / close button (toast.tsx)** and **100dvh (GardenEditorPage.tsx)**: purely presentation/layout; no untrusted data handling, no authn/authz surface. No security-relevant problems in this change. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict: No material issues found** I verified the four changes through the correctness lens: - **Journal date filter (`JournalPanel.tsx`)** — HTML date inputs emit `YYYY-MM-DD`, which is exactly the format `observed_at` is stored in (`0008_journal.sql:22`) and what the store compares against (`internal/store/journal.go:93-100`, `observed_at >= ?` / `observed_at <= ?`). String comparison is valid for this fixed-width format, and the range is inclusive on both ends as the UI implies. Empty strings are spread-conditionally omitted, so an unset bound doesn't filter — matching `JournalFilter`'s "undefined = don't filter" contract (`web/src/lib/journal.ts:32-38`). The `min`/`max` cross-constraints on the inputs only bound the picker; even if a user bypasses them, the backend `validDatePtr` check (`internal/service/journal.go:58`) still validates each value, and an inverted range simply yields an empty result rather than an error. Correct. - **Session-expiry 401 (`agent.ts`)** — The new `res.status === 401` branch sits after the abort check and before the generic `!res.ok` fallthrough, so it intercepts 401s first. `location.pathname + location.search` + `encodeURIComponent` is the right redirect-preserving construction. Matches the app's existing 401 treatment. Correct. - **Error toast persistence (`toast.tsx`)** — The effect early-returns for `isError` (no timer set) and otherwise keeps the 4s timer for info toasts; `isError` is correctly added to the dependency array. The dismiss button calls `dismiss(item.id)` for both tones. No logic bug. - **`100dvh` (`GardenEditorPage.tsx`)** — A unit/viewport-correctness swap; `calc(100dvh-8rem)` uses the same arbitrary-value syntax as the prior `100vh` form, which already compiled/applied. `dvh` reflects the dynamic viewport, which is the intended fix. Correct. No semantic, formula, or boundary issue surfaced. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Verdict: Minor issues found** - **`web/src/lib/agent.ts:168-169` — 401 redirect hand-rolls the login redirect instead of routing through the codebase's centralized `safeRedirectPath`/router pattern.** The rest of the app funnels auth redirects through `safeRedirectPath` (`web/src/lib/redirect.ts:12`, used at `web/src/router.tsx:40,59,90` and `web/src/pages/LoginPage.tsx:36`), whose header comment explicitly states "both go through this so a caller-controlled value can never send the user to an external origin." The new 401 path in `streamChat` instead builds the URL by hand (`encodeURIComponent(location.pathname + location.search)` + `window.location.assign('/login?redirect=…')`). The value is derived from `location`, so it is always same-origin and not a security hole; the issue is duplication of a deliberately centralized pattern. Note the `streamChat` function legitimately can't use TanStack Router's `redirect()` (it's not in a loader/beforeLoad and uses a raw `fetch`), so the realistic fix is just to reuse `safeRedirectPath` for the value construction rather than the router throw. Small, not blocking. - **`web/src/editor/JournalPanel.tsx:89,99` — date-input control styling duplicated verbatim with no shared class constant.** The `rounded-md border border-border bg-surface px-1.5 py-1 text-fg outline-none focus-visible:ring-2 focus-visible:ring-accent/40` string is repeated identically for the `From` and `To` inputs (lines 89 and 99), and a near-identical variant already exists for the scope `<select>` at line 69 and the `Clear` button at line 109. A small `const inputCls = '…'` at module top would keep them from drifting. Trivial cleanliness nit. - **`web/src/editor/JournalPanel.tsx:49-53` — `filter` built from three stacked ternary-spread branches.** The `...(scopeObjectId != null ? { objectId: scopeObjectId } : {})` idiom, stacked three times, reads as clever rather than clear. A `compact`/`Object.fromEntries(Object.entries({...}).filter(...))` helper or plain assignments into a `const filter: JournalFilter = {}` would scale better as more filter fields appear. Trivial maintainability nit. All three are cleanliness/maintainability nits, not blockers. The toast (error toasts don't auto-dismiss, dismiss button) and `100dvh` changes are clean and follow the surrounding patterns. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> Confirmed: `push` at line 22 has no cap, error toasts skip the `setTimeout` at line 40, and `Toaster` renders the whole array at line 73. The unbounded-growth finding survives. The second bullet is a self-described non-issue and was not in the findings JSON, so it's dropped. **VERDICT: Minor issues** - **`web/src/components/ui/toast.tsx:40` — error toasts now accumulate with no bound.** Previously every toast auto-dismissed at 4s, so the `toasts` array in the zustand store self-drained. Now error toasts never time out and are removed only by an explicit `dismiss` click. The store has no max-cap — `push` just appends (line 22: `set((s) => ({ toasts: [...s.toasts, { id: nextId++, message, tone }] }))`) — and `Toaster` renders the entire array (lines 73-75). Any path that fires repeated `toast.error` calls — a user mashing retry on a failing mutation, a background refetch storm that surfaces errors, or a stuck loop — will grow the stack and the DOM indefinitely, where before the 4s timeout provided natural GC. Suggested fix: cap the `toasts` array in `push` (e.g. drop the oldest when exceeding ~5), or at minimum drop the oldest error when an error is pushed while already at the cap — preserves "errors persist" without unbounded growth. The `100dvh` and `streamChat` 401-redirect changes are one-shot CSS/navigation changes with no performance surface. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I verified the key suspicion and it's clean. My initial concern was that the `to` date filter would be exclusive of the "To" day (a `<= 'YYYY-MM-DD'` against a timestamp would drop same-day entries). But the migration at `internal/store/migrations/0008_journal.sql:22` shows `observed_at` is stored as `'YYYY-MM-DD'` (date-only text), and `internal/store/journal.go:97-99` does `j.observed_at <= ?` against that same date string — so `to: 2025-05-15` correctly includes all May 15 entries. No boundary bug. ## Verdict: No material issues found - **Toast close-button / no-auto-dismiss for errors** (`web/src/components/ui/toast.tsx`): verified the full component. The `useEffect` early-returns for errors (no timer leak), the close button calls `dismiss(item.id)`, and the `isError` dep keeps the effect correct if tone ever changed. Clean. - **Journal date filter** (`web/src/editor/JournalPanel.tsx`): empty strings correctly don't filter (`...(from ? {from} : {})`), Clear resets both, and the `min`/`max` cross-constraints use `|| undefined` so an empty opposite bound doesn't constrain. The `to`-inclusive-of-the-day concern was disproven by reading the schema and store query above. Clean. - **401 mid-chat** (`web/src/lib/agent.ts:163-171`): the branch `return`s after handling, so it doesn't fall through into the `!res.ok` block. `encodeURIComponent` guards the redirect param. One minor, non-material observation: `onError` pushes a toast immediately before `window.location.assign` navigates away, so that toast is likely unmounted before it's seen — but the login page itself is the real signal, so this isn't a broken error path. Not worth flagging as a finding. - **`100dvh` change**: pure CSS value swap, no error-handling surface. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** I reviewed all four changes through the security lens and found no authn/authz gaps, injection vectors, unsafe redirects, or secret leakage. - **toast.tsx** — purely a UI change (manual dismiss, conditional auto-dismiss). No security impact. - **JournalPanel.tsx / journal.ts** — date strings from native `<input type="date">` are passed through the existing `api.get()` wrapper, which feeds them into `URLSearchParams`. Properly encoded, no injection path. - **agent.ts 401 redirect** — `location.pathname + location.search` is same-origin by construction, `encodeURIComponent` places it safely into the query string, and the destination is validated on the login side by `safeRedirectPath` (rejects `//` protocol-relative and non-`/` prefixes). Verified against `redirect.ts:12` and `LoginPage.tsx:36`. No open-redirect vulnerability. - **GardenEditorPage.tsx** — CSS viewport unit swap, zero security relevance. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** Through the Correctness lens, the four changes are all well-reasoned and free of logic bugs: - **`100dvh`** — The dynamic viewport unit correctly fixes the mobile Safari/Chrome overflow issue described; `100vh` on mobile browsers includes the space behind the URL bar, while `100dvh` tracks the actual visible viewport. - **Toast persistence** — The `isError` guard correctly skips `setTimeout` only for error toasts, preserving existing behavior for info toasts. The `useEffect` dependency list includes `isError`, so toggling a toast's tone would correctly reset/clear timers. - **Journal date filter** — Verified that `JournalFilter` in `web/src/lib/journal.ts:36-37` already declares `from?: string` and `to?: string`, and `useJournal` spreads the filter into API params. The UI inputs use HTML `min`/`max` to prevent invalid ranges, and empty strings are correctly filtered out before being passed to the query. - **401 handling in chat** — Checked that the rest of the app (router `requireAuth` at `web/src/router.tsx:40`, `meQueryOptions` at `web/src/lib/auth.ts:35`) treats 401 as unauthenticated and redirects to `/login` with a `redirect` search param. The new agent code follows the same convention, using `encodeURIComponent` on `location.pathname + location.search`, which matches the router's pattern. No semantic errors, miscalculations, or boundary-condition mishandling were found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **No material issues found** Reviewed through the **code cleanliness & maintainability** lens. The changes are concise, well-commented, and consistent with surrounding code: - **toast.tsx** — Extracting `isError` avoids repeating `item.tone === 'error'` three times; the close button follows the same styling idiom (`focus-visible:ring-2 focus-visible:ring-accent/40`) used elsewhere. - **JournalPanel.tsx** — The date inputs reuse the existing inline styling pattern (`border-border bg-surface … focus-visible:ring-accent/40`) already present in the sibling `<select>` and other editor components. Using raw `<input>` instead of the project's `TextField` component is correct here because `TextField` renders its own `<label>` wrapper, whereas these inputs need tiny inline "From"/"To" labels. - **agent.ts** — The 401 check is placed correctly between the network-failure path and the general `!res.ok` path, keeping the error-handling flow linear and readable. - **GardenEditorPage.tsx** — One-line CSS change with a clear explanatory comment. Nothing to flag for duplication, naming, structure, or pattern drift. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **Verdict: No material issues found** I examined all four changes through the performance lens and found nothing material. - **Mobile viewport (`100dvh`)** — CSS unit swap. `dvh` updates as browser chrome shows/hides; this is expected layout work and no regression over `100vh`. - **Session expiry mid-chat** — Single synchronous path: fire `onError` callback then navigate. No loop, no allocation hotspot, no blocking I/O beyond the single redirect. - **Error toasts persist** — The toast store uses small objects and array clones on push/dismiss. Error toasts can now accumulate without auto-dismiss, which is unbounded in principle, but bounded by user action in practice and the memory footprint per toast is negligible. This is a deliberate UX tradeoff, not an efficiency regression. - **Journal date filter** — `from`/`to` are `<input type="date">` state changes (not keystroke-streaming), so each picker selection yields exactly one state update → one React Query key change → one fetch. The `filter` object is reconstructed every render, but React Query serializes query keys structurally, so identical values do not trigger spurious refetches. The `pages.flatMap((p) => p.entries)` on every render is O(total loaded entries) and only used for rendering, with no downstream hook-dependency cascade. All clean from a performance perspective. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **No material issues found** - **toast.tsx**: The early-return for `isError` in the `useEffect` correctly prevents auto-dismiss of error toasts, while info toasts still time out. No swallowed cleanup — the existing `clearTimeout(t)` teardown is preserved. The `isError` dependency is stable and correct. - **JournalPanel.tsx**: Empty-string initialization for `from`/`to` is safely handled by conditional spreading (`...(from ? { from } : {})`). The `Clear` button resets both states. Browser-native `min`/`max` constraints on the date inputs guard against invalid ranges. No nil-dereference issues in the filter construction or `entries` fallback. - **agent.ts**: The 401 check precedes the generic `!res.ok` handler, so the correct error path is taken. The `signal?.aborted` check in the `fetch` catch block already filters out user-initiated aborts before the 401 logic is reached. `window.location.assign` is used for the hard redirect, matching the rest of the app's 401 treatment. No unhandled edge cases in the session-expiry flow. </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** I traced every tainted input path in this diff through to its sink and verified each is either constrained or already protected by an existing layer: - **Journal date filter** (`web/src/editor/JournalPanel.tsx:47-53`): `from`/`to` come from `<input type="date">` and flow to `JournalFilter` → query params → `service.JournalQuery.From/To`. I verified at `internal/service/journal.go:58` that `validDatePtr` (defined at `internal/service/plantings.go:309`) parses each value with `time.Parse(dateLayout, …)` and rejects anything that isn't `YYYY-MM-DD` with `ErrInvalidInput` (→ 400). The store layer (`internal/store/journal.go:93-99`) binds them as parameterized SQL args (`args = append(args, *f.From)`), not string interpolation, so no SQL injection even for a value that slipped past. The malformed-date 400 path is covered by `TestJournalFiltersAPI` at `internal/api/journal_test.go:131-134`. No XSS: the values render only as the input's own `value` and as React-escaped list text. - **Agent 401 redirect** (`web/src/lib/agent.ts:168-169`): `back = encodeURIComponent(location.pathname + location.search)` is the *current* page's own path/query (inherently same-origin) and is only ever consumed by `/login`, which I verified at `web/src/pages/LoginPage.tsx:36` routes it through `safeRedirectPath` (`web/src/lib/redirect.ts:12-13`) — that function rejects anything not starting with a single `/` and explicitly blocks protocol-relative `//evil.com`. No open-redirect surface; the redirect target is both attacker-uncontrollable (it's the page you're already on) and validated. - **Toast** (`web/src/components/ui/toast.tsx`): `item.message` is rendered as React text children (`<span className="flex-1">{item.message}</span>`), so it's auto-escaped — no HTML injection from error messages. No auth/input concerns. - **`100dvh`** (GardenEditorPage): CSS-only, no security surface. Nothing in the security lens is materially wrong. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I'll verify each change against the actual checked-out code through the correctness lens. Now let me verify how the backend interprets `from`/`to` — the UI sends `YYYY-MM-DD` strings, and I need to confirm the backend treats them as expected (inclusive date bounds, not strict less-than). Now I want to verify the `streamChat` 401 path more carefully — specifically whether `handlers.onError` triggers any UI that races with the `window.location.assign`. Let me check how the chat UI uses `onError`. Now let me verify the JournalPanel — check that the date filter is actually wired correctly with the `useJournal` hook, and confirm the `from`/`to` are sent as query params properly. Let me also check the `location` global usage in agent.ts. Now I have enough to assess correctness. Let me trace through the key concerns: **1. Toast — error persistence:** The `useEffect` returns early when `isError` (no timer set). Close button calls `dismiss`. Logic is sound. The dependency array includes `isError` which is fine. **2. Agent 401 handling:** On 401, calls `handlers.onError` then `window.location.assign`. The `onError` sets error state and triggers `refresh.everything()` (which invalidates queries). The navigation will happen, so the React state update is moot but harmless. The redirect uses `location.pathname + location.search` encoded. This matches the router's `?redirect=` convention. One concern: `location` is referenced bare (line 168) — in browser this is `window.location`, works fine. The `redirect` param is read back by LoginPage via `safeRedirectPath` which validates same-origin path. Good. **3. Journal date filter:** Sends `from`/`to` as `YYYY-MM-DD` strings. Backend `store/journal.go:93-99` does `observed_at >= ?` and `observed_at <= ?` with string comparison. Since `observed_at` is stored as `YYYY-MM-DD` (per migration 0008), lexicographic string comparison of `YYYY-MM-DD` is correct chronological comparison. Inclusive on both ends. UI `max`/`min` constraints prevent `from > to`. Correct. **4. 100dvh:** CSS change, no logic implications for correctness lens. Let me double check one thing — whether the `from`/`to` empty-string handling in `buildUrl` (api.ts line 52) properly omits them. Yes, `v === ''` is skipped. And the spread `...(from ? { from } : {})` means empty string is falsy so `from` key is omitted entirely. Consistent. Let me verify the backend `validDatePtr` to ensure the format matches what the UI sends. I've verified all four changes through the correctness lens. My findings: **VERDICT: No material issues found** - **Toast persistence (`toast.tsx`):** The `useEffect` early-returns when `isError`, so no timer is set — errors stay until the ✕ button calls `dismiss`. `isError` is derived from the immutable `item.tone`, so it never flips for a given item; the dep-array addition is harmless. Verified by reading the full file. - **Agent 401 (`agent.ts:163-171`):** Fires `onError` then `window.location.assign` to `/login?redirect=<encoded path>`. The encoded `location.pathname + location.search` matches the `?redirect=` search param the router (`router.tsx:84-86`) and LoginPage (`LoginPage.tsx:36` via `safeRedirectPath`) expect; `safeRedirectPath` rejects `//evil.com` / absolute URLs, and a same-origin path always starts with `/`, so the value is safe. The `onError`→`refresh.everything()` invalidations racing with navigation are wasted work, not a correctness bug. Verified `ApiError.isUnauthorized` exists at `api.ts:30` for parity with the rest of the app. - **Journal date filter (`JournalPanel.tsx`):** UI emits `YYYY-MM-DD` strings (HTML date input format). Backend validates with `time.Parse("2006-01-02", …)` (`plantings.go:309-314`) and stores `observed_at` as `YYYY-MM-DD` (`migrations/0008_journal.sql:22`), compared as `>=`/`<=` string bounds (`store/journal.go:93-99`) — lexicographic order on `YYYY-MM-DD` IS chronological, inclusive on both ends. The `max={to || undefined}` / `min={from || undefined}` constraints prevent an inverted range in the picker. Empty strings are falsy so the spread omits the key, and `buildUrl` (`api.ts:52`) also skips `''` — double-guarded. The inline `filter` object is rebuilt each render, but that was already true before this change and TanStack Query does structural key comparison. Verified end-to-end. - **100dvh (`GardenEditorPage.tsx`):** Pure CSS substitution, `100dvh` is the correct dynamic-viewport unit for the stated mobile-Safari/Chrome problem; no logic impact. No semantic, formula, unit, or boundary issues found in this lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Confirmed: PublicGardenPage.tsx:56 still uses `calc(100vh-8rem)` and is the only remaining call site of that pattern, while the PR fixed the equivalent layout in GardenEditorPage to `100dvh`. The finding survives. The other two draft findings were self-described as non-issues ("this is fine", "a consistency/maintainability nit") and weren't carried as findings in the JSON anyway. Only the PublicGardenPage inconsistency survives. VERDICT: Minor issues - **`web/src/pages/PublicGardenPage.tsx:56`** — The `100dvh` fix is applied only to the authenticated editor. `PublicGardenPage.tsx:56` still uses `calc(100vh-8rem)` for the same full-height layout on the same class of mobile browsers — an unauthenticated visitor on mobile Safari hits exactly the bug the PR says it's fixing (canvas bottom / Fit button pushed under browser chrome when the URL bar is visible). Verified by grep: this is the only remaining `calc(100…-8rem)` call site, and the PR only touched `GardenEditorPage`. Either the fix is wrong for one of the two pages, or `PublicGardenPage` was missed. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Through the Performance lens, I examined each change for refetch storms, unbounded growth, allocations on hot paths, and unnecessary re-renders. Verifying against the checked-out code: - **Journal filter object** (`JournalPanel.tsx:49-53`): A new `filter` object is built each render and feeds `useJournal`'s `queryKey`. I checked `journal.ts:42-58` — the key is `['gardens', gardenId, 'journal', filter]`. TanStack Query hashes keys structurally (`hashKey` does deep comparison), so an equal-valued new object does **not** trigger a refetch. This was already the pattern before the PR (the old line created `{ objectId }` / `{}` inline too), so no regression. Date inputs fire `onChange` once per complete date selection, not per keystroke, so each `from`/`to` change is one refetch — no storm, no debounce needed. - **Error toast persistence** (`toast.tsx:40-43`): Errors no longer auto-dismiss, so they accumulate until dismissed. I checked there's no automatic retry loop that would pump errors in the background (mutations don't configure `retry`, and `streamChat` doesn't loop). Accumulation is bounded by user-initiated actions and each toast is individually dismissible, so this is not an unbounded-growth concern. - **streamChat 401 branch** (`agent.ts:163-171`): `window.location.assign` navigates away — no path that does repeated work. - **`100dvh`** and toast layout changes: pure CSS/render, no perf impact. No material issues found in my lane. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I'll examine the changed files and their surrounding context through the error-handling/edge-case lens. Now let me check how the rest of the app handles 401 redirects and the ApiError.unauthorized pattern to compare with the agent.ts change. Let me check the streamChat callers and whether the 401 navigation interacts with any abort/cleanup logic. Now let me check how the login page reads the redirect param, to verify the agent.ts redirect is consistent and won't be lost. Now I have full context. Let me think through the edge cases for each change. **Toast change:** Error toasts no longer auto-dismiss; only info ones time out. The `useEffect` early-returns for errors. The dep array includes `isError` (stable per item). One concern: an error toast that never auto-dismisses — if a user walks away with many failed mutations, toasts accumulate. But there's now a ✕ close button, so dismissible. This is intentional per the PR. No swallowed error. Fine. One subtle issue: the close button is rendered for *both* info and error toasts. For an info toast that's about to auto-dismiss at 4s, the button is harmless. Fine. **Journal date filter:** `from`/`to` are raw strings from `<input type="date">`. Browsers emit `YYYY-MM-DD` or empty string. Empty string is falsy → omitted from filter. The `max={to || undefined}` / `min={from || undefined}` constraints prevent inverted ranges in the UI. But there's no validation that `from <= to` if a user manually... actually with `max`/`min` constraints the native picker enforces it. Edge case: if `to` is set and user clears `to` via Clear button, `from`'s `max` becomes undefined — fine. One real concern: the filter object is reconstructed every render as a new object literal. `useJournal` uses it as a query key (via `journalKey` → `['gardens', gardenId, 'journal', filter]`). TanStack Query serializes the key; a new object with same contents hashes equal, so no infinite refetch. But `api.get` spreads `...filter` into params — `{ from: '2026-01-01', to: undefined }`? No — empty strings are omitted via the spread `...(from ? {from} : {})`, so undefined never appears. The `buildUrl` also skips `''`. Good. No issue. **Agent 401 handling:** On 401, calls `onError` then `window.location.assign('/login?redirect=...')`. The `onError` sets `setPending(null)` and `setError(message)` in ChatPanel, then `refresh.everything()`. But immediately after, `window.location.assign` navigates away — the whole page unloads. So the `setError`/`setPending` state updates are moot (page is going away). Not a bug, just dead work. A more material concern: `onError` is called *before* the navigation, and `ChatPanel.onError` calls `refresh.everything()` which fires API queries — those will race with the navigation. But navigation unloads everything; the in-flight queries get cancelled by the browser. Not a real problem. Is there an issue that `window.location.assign` happens during render cycle / async fetch resolution? It's in an async function after `await fetch`, so it's in a microtask — fine, navigation is allowed. The redirect uses `location.pathname + location.search` — but the chat is on the garden editor page; hash is dropped. Minor, not material. The `encodeURIComponent` is correct. One thing: this bypasses `safeRedirectPath` validation on the *writing* side (the agent.ts constructs the redirect), but the *reading* side (LoginPage) runs it through `safeRedirectPath`, so it's sanitized on consumption. Since `location.pathname` always starts with `/` and is same-origin, it's safe. Fine. **100dvh change:** Pure CSS, no error path. `dvh` has good support. Fallback: browsers that don't support `dvh` will ignore the declaration... actually no, if `calc(100dvh-8rem)` is invalid in an old browser, the whole `h-[...]` class is invalid and height falls back to auto/content. That could collapse the editor layout on very old browsers. But Tailwind arbitrary values — `h-[calc(100dvh-8rem)]` — produce `height: calc(100dvh-8rem)`. Old browsers ignoring `dvh` → invalid → height unset. This is a potential regression for old browser support, but it's a deliberate tradeoff per the PR. Not an error-handling issue per se. Outside my lens really. Let me check if there's any issue with the date filter when the journal query errors (e.g., 400 from malformed date). The browser `<input type="date">` always emits valid `YYYY-MM-DD` or empty, so malformed dates can't be sent from this UI. Good — the existing 400 test for `?from=nonsense` won't be triggered from this UI. I've found nothing material in my lens. Let me double check one thing: the toast `useEffect` early return — when `isError` is true, the effect returns `undefined` (no cleanup). When the component re-renders with same `isError`, effect doesn't re-run (deps unchanged). Fine. When an info toast transitions... tone is fixed per toast (set at push), so `isError` never changes for a given item. Fine. Verdict: No material issues found in the error-handling/edge-case lens. The changes are careful — empty-string falsiness is handled, the 401 path navigates correctly, error toasts are dismissible, and the existing 400-on-malformed-date backend validation is unreachable from this UI because native date inputs constrain the format. Let me verify one last thing — that there's no path where a 401 during streaming (not initial fetch) needs handling. The 401 check is only on the initial response, before the read loop. Mid-stream 401 can't happen (response already 200). Fine. No material issues found.**VERDICT: No material issues found** I reviewed all four changed files through the error-handling/edge-case lens, verifying against the checked-out code (not the diff alone): - **`toast.tsx`** — Error toasts early-return from the timeout effect; `isError` is fixed per item (tone set at `push`), so it never flips and re-triggers the effect. The ✕ button works for both tones. Info toasts still time out at 4s. No swallowed dismissal, no accumulation-without-escape (close button exists). Verified by reading the full file. - **`JournalPanel.tsx` date filter** — Empty `from`/`to` are falsy and omitted via conditional spread, so they never reach the query as `undefined`/`''` params. `max={to || undefined}`/`min={from || undefined}` prevent inverted ranges at the input level. The backend's `?from=nonsense → 400` path (verified in `internal/api/journal_test.go:131`) is unreachable from this UI because native `<input type="date">` only emits `YYYY-MM-DD` or empty — never arbitrary strings. The reconstructed-each-render filter object is fine: TanStack serializes query keys by value, so no spurious refetch. Verified against `lib/journal.ts` (`JournalFilter`, `useJournal`, `buildUrl`'s `''`/`null`/`undefined` skip) and the store layer. - **`agent.ts` 401** — Placed correctly: after the fetch try/catch, before the generic `!res.ok` branch, so a 401 is not swallowed into the "assistant unavailable" message. `onError` fires before `window.location.assign`, so `ChatPanel`'s `setPending(null)`/`refresh.everything()` run but are moot (page unloads). The redirect target `location.pathname + location.search` is always same-origin and starts with `/`, and `LoginPage` runs it through `safeRedirectPath` on consumption (`pages/LoginPage.tsx:36`), so it's sanitized at read time. `encodeURIComponent` is applied. Verified against `ChatPanel.tsx:84` and `lib/redirect.ts`. - **`GardenEditorPage.tsx` `100dvh`** — Pure CSS substitution; no error path introduced. (Old-browser fallback for unsupported `dvh` units is a layout/compat concern, outside this lens.) No unhappy paths are mishandled, no errors swallowed, no missing cleanup. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — 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, I verified each change against the live code: - **401 redirect in chat** (`web/src/lib/agent.ts:167-169`): The redirect target is `encodeURIComponent(location.pathname + location.search)`, which is then consumed by the login page's `safeRedirectPath` (`web/src/lib/redirect.ts:12-14`). That helper rejects protocol-relative (`//evil.com`) and absolute URLs, so even a manipulated query string cannot turn this into an open redirect. The flow matches the existing `ApiError.isUnauthorized` → `/login?redirect=` pattern used by the router guards. - **Journal date filter** (`web/src/editor/JournalPanel.tsx:47-53`): `from`/`to` strings pass through to `api.get` with parameterized query encoding (`URLSearchParams`). The backend (`internal/api/journal.go:59-63`) forwards them to the service, which validates them via `validDatePtr` (`internal/service/plantings.go:309-315`) against the strict `2006-01-02` layout. The store (`internal/store/journal.go:93-99`) binds them as `?` placeholders, so SQL injection is not possible. - **Toast persistence & close button** (`web/src/components/ui/toast.tsx`): The message is rendered as a React text node inside a `<span>`, which is inherently escaped. No new injection surface is introduced. Nothing in this diff creates or widens an authn/authz gap, injection vector, unsafe deserialization path, secret leak, or missing input-validation hole. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict: No material issues found** Through the correctness lens, I scrutinized each change for logic bugs, semantic errors, and incorrect behavior: **Mobile viewport (`100dvh`)** — Verified `GardenEditorPage.tsx:405`. `100dvh` correctly uses the dynamic viewport height, fixing the mobile Safari/Chrome issue where `100vh` includes the URL bar-hidden area. This is the correct CSS fix for the described problem. **Session expiry in chat (`401` handling)** — Verified `web/src/lib/agent.ts:163-171`. The 401 check is placed correctly after the fetch response and before the SSE body read. I traced the redirect URL construction (`encodeURIComponent(location.pathname + location.search)`) through to `LoginPage.tsx:36` and `safeRedirectPath`. TanStack Router decodes `%2F` back to `/` when parsing search params, so `safeRedirectPath` receives `/gardens/...` and accepts it. The behavior matches the existing router-guard 401 pattern exactly. No logic error. **Sticky error toasts** — Verified `web/src/components/ui/toast.tsx:36-43`. The `useEffect` correctly skips the auto-dismiss timer when `isError` is true. Adding `isError` to the dependency array is correct because it's read inside the effect. The close button properly calls `dismiss(item.id)`. No behavioral bug introduced. **Journal date filter** — Verified `web/src/editor/JournalPanel.tsx:47-114` and backend `internal/store/journal.go:93-99` / `internal/service/journal.go:54-85`. The UI passes `from`/`to` strings directly to the existing `JournalFilter`/`useJournal` API. Empty strings are excluded from the filter via conditional spreads, and `api.ts:52` strips empty-string params from the query string. The backend uses inclusive `>=` / `<=` on `observed_at` with `YYYY-MM-DD` string comparison, which is correct for ISO date ordering. HTML `min`/`max` attributes provide native browser validation. No backend change means no risk of store or service-layer regressions. I also verified that `tsc --noEmit` passes per the PR description, and the code structure is consistent with existing patterns. No material correctness issues were found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run returned no output </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found. These changes are surface-level UX fixes (viewport unit, error message wording, toast lifetime, date inputs). None touch hot loops, query patterns, or allocation volume in a material way. - **`toast.tsx`** — Adding `isError` to the `useEffect` dep array is correct; it does not introduce extra effect runs because `item.id` and `dismiss` are stable, and the effect still bails early for errors. Persisting error toasts in the DOM is bounded by user-initiated actions, not background polling. - **`JournalPanel.tsx`** — The inline `filter` object is recreated every render, but the old code already used the same pattern (`scopeObjectId`); TanStack Query serializes keys, so this does not trigger spurious refetches. Date inputs commit values discretely, so state churn is low. - **`agent.ts`** — The new `401` branch is a single synchronous check before the existing stream handling. No I/O or allocation change. - **`GardenEditorPage.tsx`** — `100dvh` vs `100vh` is a CSS string change with no runtime work implications. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found. - **`web/src/lib/agent.ts`:** The 401 handling correctly intercepts the status before the generic `!res.ok` branch. The redirect URL construction (`encodeURIComponent(location.pathname + location.search)`) preserves the current path and query, matching the app's existing login-redirect convention (`safeRedirectPath`). `window.location.assign` after `handlers.onError` means the React state update from `setError` may still flush briefly before navigation, but this is harmless and consistent with how the rest of the app handles hard redirects. - **`web/src/components/ui/toast.tsx`:** Error toasts skip the timeout; the effect cleanup correctly clears any pending timer on unmount. The close button safely calls `dismiss(item.id)`, which filters the store array. - **`web/src/editor/JournalPanel.tsx`:** Empty strings are correctly omitted from the filter object via spread conditionals. The `min`/`max` attributes on the date inputs are guarded with `|| undefined`, which React correctly renders as absent. The `type="date"` input ensures only valid `YYYY-MM-DD` values reach state. - **`web/src/pages/GardenEditorPage.tsx`:** The `100dvh` change is a CSS progressive enhancement for mobile browsers; no error-handling implications. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-22 03:53:19 +00:00
Address Gadfly findings on #85
Build image / build-and-push (push) Successful in 16s
757ac7394d
- PublicGardenPage had the same 100vh mobile bug I fixed in the editor — 100dvh
  there too, so the fix is consistent across both full-height pages.
- Cap the toast stack at 4 (drop oldest). Now that error toasts don't auto-
  dismiss, a burst of failures could otherwise grow the stack unbounded and push
  the newest — the one that just happened — off-screen.
- Hoist the From/To date-input styling to a shared dateInputClass const so the
  two don't drift.

Not taken: reusing safeRedirectPath for the 401 redirect (it validates an
incoming redirect param, it doesn't build the outgoing one — the two uses don't
overlap), and refactoring the three-branch filter spread (it's clear at three
fields; a helper earns its keep when there are more).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve merged commit 1e2b763566 into main 2026-07-22 03:54:03 +00:00
Sign in to join this conversation.