The top bar was desktop-shaped — logo + Gardens/Plants/Settings + name + Sign out all in one row, wrapping "Sign out" to two lines at 390px, with no thumb-reachable navigation.
Mobile-first now
Slim top bar: brand (left — still the way back to /gardens, which matters in the editor) + a compact account control (right).
Bottom tab bar for the sections (Gardens / Plants / Settings-admin) in the thumb zone, env(safe-area-inset-bottom)-aware, ≥52px targets, shown only when signed in.
Account/sign-out is a small top-right popover ("Signed in as …" / Sign out) — sign out in 2 taps. Closes on an outside tap via a backdrop button, no document listener.
Desktop (md:+) keeps the inline top nav; the bottom bar is md:hidden.
The editor is a full-screen context, so it owns the bottom of the screen — the app bottom bar hides on /gardens/$gardenId (via useMatchRoute), so nothing competes there. The editor's own mode bar arrives in #99; until then the brand link is the way back.
Verified live
390px: bottom bar on the gardens list, account menu opens, and it's absent in the editor (confirmed the nav[aria-label="Sections"] isn't rendered there) with the brand back-link present.
1280px: inline top nav unchanged.
tsc clean; vitest 87 pass; npm run build green.
Screenshots (mobile list w/ bottom bar, account menu, desktop) captured during review.
Closes #98. Part of #96.
The top bar was desktop-shaped — logo + Gardens/Plants/Settings + name + Sign out all in one row, wrapping "Sign out" to two lines at 390px, with no thumb-reachable navigation.
## Mobile-first now
- **Slim top bar:** brand (left — still the way back to `/gardens`, which matters in the editor) + a compact account control (right).
- **Bottom tab bar** for the sections (Gardens / Plants / Settings-admin) in the thumb zone, `env(safe-area-inset-bottom)`-aware, ≥52px targets, shown only when signed in.
- **Account/sign-out** is a small top-right popover ("Signed in as …" / Sign out) — sign out in 2 taps. Closes on an outside tap via a backdrop button, no document listener.
- **Desktop (`md:+`)** keeps the inline top nav; the bottom bar is `md:hidden`.
- **The editor is a full-screen context**, so it owns the bottom of the screen — the app bottom bar hides on `/gardens/$gardenId` (via `useMatchRoute`), so nothing competes there. The editor's own mode bar arrives in #99; until then the brand link is the way back.
## Verified live
- 390px: bottom bar on the gardens list, account menu opens, and it's **absent in the editor** (confirmed the `nav[aria-label="Sections"]` isn't rendered there) with the brand back-link present.
- 1280px: inline top nav unchanged.
`tsc` clean; `vitest` 87 pass; `npm run build` green.
Screenshots (mobile list w/ bottom bar, account menu, desktop) captured during review.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The top bar was desktop-shaped — logo + Gardens/Plants/Settings + name +
Sign out crammed in one row, wrapping "Sign out" to two lines at 390px,
with no thumb-reachable navigation.
Mobile-first now:
- Slim top bar: brand (left, still the way back to /gardens) + a compact
account control (right).
- Section nav (Gardens/Plants/Settings) moves to a bottom tab bar in the
thumb zone, safe-area-aware, ≥52px targets, shown only when signed in.
- Account/sign-out is a small top-right popover (Signed in as … / Sign
out), reachable in 2 taps. Close-on-outside-tap via a backdrop button,
no document listener.
- Desktop (md:+) keeps the inline top nav; the bottom bar is md:hidden.
- The editor is a full-screen context, so it owns the bottom of the
screen — the app bottom bar hides on /gardens/$id (via useMatchRoute),
leaving no competing bars there; the editor's own mode bar arrives in
the mode-model issue.
Verified live at 390px and 1280px: bottom bar on the list, hidden in the
editor, account menu opens, desktop unchanged. tsc + vitest green.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 05:02:11Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **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** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **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>
Gadfly findings on #98:
- Logout failure (4 models): the catch called setOpen(false), closing the
popover and hiding the only "Retry sign out" affordance — contradicting
its own comment. Keep the popover open on failure so the retry button
(driven by logout.isError) stays on screen.
- Public garden (2 models): the bottom-bar suppression missed /g/$token,
whose PublicGardenPage also renders a 100dvh-8rem canvas — a signed-in
viewer of a shared link got the bar overlapping it. Suppress there too.
- BottomNav base className carried text-muted, fighting the active
text-accent-strong and violating the file's own "color only in state
props" convention (3 models). Moved color entirely to the state props.
- BottomNav sections prop type was a hand-written partial copy of the
sections shape; derive `Section = (typeof sections)[number]` (2 models).
- Popover open state now resets on route change, so navigating (bottom nav
or browser back) can't strand an invisible full-screen backdrop.
- Coupled the <main> bottom clearance to BottomNav's height (both 3.5rem /
h-14, co-located with a note) and switched the template-string className
to cn().
Verified live: route-change closes the popover with no lingering backdrop.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰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>
🟠Logout failure closes the popover, hiding the only retry/error affordance with no toast fallback
correctness, error-handling, maintainability · flagged by 4 models
web/src/components/layout/AppShell.tsx:113 — On logout failure, onLogout's catch block calls setOpen(false), closing the account popover. That popover is the only place the failure is surfaced (title="Sign out failed — try again" and the "Retry sign out" label live inside it, at lines 159/162), and the comment directly above the call still says "leave the user where they are (the button re-enables for a retry)" — the code no longer does that; it hides the retry button instead. `use…
🪰 Gadfly · advisory
🟠 **Logout failure closes the popover, hiding the only retry/error affordance with no toast fallback**
_correctness, error-handling, maintainability · flagged by 4 models_
- **`web/src/components/layout/AppShell.tsx:113`** — On logout failure, `onLogout`'s catch block calls `setOpen(false)`, closing the account popover. That popover is the only place the failure is surfaced (`title="Sign out failed — try again"` and the "Retry sign out" label live inside it, at lines 159/162), and the comment directly above the call still says "leave the user where they are (the button re-enables for a retry)" — the code no longer does that; it hides the retry button instead. `use…
<sub>🪰 Gadfly · advisory</sub>
🟡BottomNav sections prop type is a hand-written partial copy of the sections array shape, prone to drift
maintainability · flagged by 2 models
web/src/components/layout/AppShell.tsx:175 — BottomNav's sections prop is typed by hand as ReadonlyArray<{ to: string; label: string; icon: string }> rather than derived from the actual source of truth, the sections array at lines 8-12 (typeof sections[number] or a Pick). Confirmed: the real array also carries adminOnly, so this is already a partial, hand-copied duplicate of the shape — the kind of thing that silently drifts when a field is added.
🪰 Gadfly · advisory
🟡 **BottomNav sections prop type is a hand-written partial copy of the sections array shape, prone to drift**
_maintainability · flagged by 2 models_
- `web/src/components/layout/AppShell.tsx:175` — `BottomNav`'s `sections` prop is typed by hand as `ReadonlyArray<{ to: string; label: string; icon: string }>` rather than derived from the actual source of truth, the `sections` array at lines 8-12 (`typeof sections[number]` or a `Pick`). Confirmed: the real array also carries `adminOnly`, so this is already a partial, hand-copied duplicate of the shape — the kind of thing that silently drifts when a field is added.
<sub>🪰 Gadfly · advisory</sub>
🟡Second backdrop-blur layer adds mobile scroll/compositor cost
performance · flagged by 1 model
web/src/components/layout/AppShell.tsx:180 — The mobile bottom nav adds backdrop-blur alongside the header's existing blur, creating two simultaneous expensive compositor layers on mobile. During scroll, the browser must continuously capture and blur content behind both fixed elements, which can cause frame drops and increased battery usage on lower-end devices. The visual benefit is marginal because bg-surface/95 is already nearly opaque. Fix: Remove backdrop-blur from the bot…
🪰 Gadfly · advisory
🟡 **Second backdrop-blur layer adds mobile scroll/compositor cost**
_performance · flagged by 1 model_
- `web/src/components/layout/AppShell.tsx:180` — The mobile bottom nav adds `backdrop-blur` alongside the header's existing blur, creating **two simultaneous expensive compositor layers** on mobile. During scroll, the browser must continuously capture and blur content behind both fixed elements, which can cause frame drops and increased battery usage on lower-end devices. The visual benefit is marginal because `bg-surface/95` is already nearly opaque. **Fix:** Remove `backdrop-blur` from the bot…
<sub>🪰 Gadfly · advisory</sub>
🔴BottomNav active tab has conflicting text-muted/text-accent-strong classes
correctness, maintainability · flagged by 3 models
web/src/components/layout/AppShell.tsx:187 — The BottomNavLink base className includes text-muted, but activeProps only adds text-accent-strong. The file's own comment (line 14-16) explains that TanStack Router concatenates base + state classes, so conflicting color utilities must live only in activeProps/inactiveProps to avoid ambiguous overrides. Because text-muted is present in the base string, both classes end up on the active tab element; whichever Tailwind emit…
🪰 Gadfly · advisory
🔴 **BottomNav active tab has conflicting text-muted/text-accent-strong classes**
_correctness, maintainability · flagged by 3 models_
- **`web/src/components/layout/AppShell.tsx:187`** — The `BottomNav` `Link` base `className` includes `text-muted`, but `activeProps` only adds `text-accent-strong`. The file's own comment (line 14-16) explains that TanStack Router concatenates base + state classes, so conflicting color utilities must live *only* in `activeProps`/`inactiveProps` to avoid ambiguous overrides. Because `text-muted` is present in the base string, both classes end up on the active tab element; whichever Tailwind emit…
<sub>🪰 Gadfly · advisory</sub>
Server-side enforcement is intact — this is purely a client-side nav/UI change over React, with text content auto-escaped (no dangerouslySetInnerHTML), no new API calls, no new user-controlled data flowing into URLs, storage, or templates.
VERDICT: No material issues found
The admin-only filtering (AppShell.tsx:38) is UI convenience only; verified the server still gates /settings behind requireAuth() + requireAdmin() in internal/api/api.go:158 and internal/api/settings.go:22, matching the pre-existing pattern the removed comment described. No authz regression.
displayName is rendered as plain JSX text (AppShell.tsx:128,152), not via dangerouslySetInnerHTML or a URL/attribute sink — React escapes it, so no XSS vector even though it's user-controlled.
The click-away backdrop (AppShell.tsx:141-146) and popover use local component state only; no new storage, no new endpoints, no tokens or secrets touched.
Route matching (useMatchRoute) and section list are static, hard-coded arrays — no injection surface.
Nothing here changes trust boundaries, request handling, or credential flow — it's a pure client-side layout/nav refactor.
🎯 Correctness — Minor issues
Both findings confirmed against the actual code. publicGardenRoute (g/$token) is a child of rootRoute whose component is AppShell, has no auth guard, and PublicGardenPage.tsx:56 hardcodes the same h-[calc(100dvh-8rem)] as the editor — but AppShell.tsx:35 only exempts gardens/$gardenId from showBottomNav, so a signed-in user on their own share link gets the overlay. The onLogout catch block does call setOpen(false) at line 113, closing the popover that shows the retry state described in the adjacent comment.
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:35 — Bottom-nav suppression (inEditor) only matches gardens/$gardenId; it doesn't exempt g/$token, which is also a full-screen canvas route (router.tsx:137-141, nested under AppShell via rootRoute, no auth guard so a signed-in user keeps their session there). PublicGardenPage.tsx:56 hardcodes h-[calc(100dvh-8rem)], the same calc GardenEditorPage.tsx:405 uses but is correctly exempted for via inEditor. For a signed-in user on their own share link, showBottomNav becomes true, adding pb-[calc(4.5rem+env(safe-area-inset-bottom))] to main and a fixedBottomNav (z-20) that the page's fixed-height canvas doesn't leave room for — the bottom of the pan/zoom canvas ends up under the tab bar. Fix: fold g/$token into the full-screen check, or make the page's height calc account for the bottom bar.
web/src/components/layout/AppShell.tsx:113 — onLogout's catch branch calls setOpen(false), closing the account popover on a failed sign-out. The comment directly above says the intent is to leave the user in place so "the button re-enables for a retry," but closing the popover hides the "Retry sign out" label (driven by logout.isError, rendered at line 162) until the user reopens the menu — an extra tap contrary to the comment's stated intent. Fix: drop setOpen(false) in the catch branch.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:113 — In AccountMenu's onLogout, the catch-block comment ("leave the user where they are ... rather than pretending they're signed out") is directly contradicted by the setOpen(false) on the next line: a failed sign-out closes the account popover, hiding the logout.isError-driven "Retry sign out" state the comment describes. The user has to reopen the menu to retry. Either the behavior should not close the popover on error, or the comment needs to be rewritten to describe why closing is still fine — as written it's stale/misleading, which CLAUDE.md flags as worse than no comment. Verified by reading the full function (lines 105-115).
web/src/components/layout/AppShell.tsx:85 (clearance) vs. :180,187 (bar height) — <main>'s bottom padding is a hardcoded pb-[calc(4.5rem+env(safe-area-inset-bottom))], while the actual BottomNav height is built from separate, independently-chosen values (min-h-[3.25rem] per link, py-1.5, border-t, plus its own pb-[env(safe-area-inset-bottom)]). Nothing ties these two together — confirmed by reading both blocks. If BottomNav's height changes later, it's easy to forget the matching pb- update in AppShell, silently reintroducing the "content hidden behind the bar" bug this PR fixes. Worth a shared constant or a cross-referencing comment.
web/src/components/layout/AppShell.tsx:175 — BottomNav's sections prop is typed by hand as ReadonlyArray<{ to: string; label: string; icon: string }> rather than derived from the actual source of truth, the sections array at lines 8-12 (typeof sections[number] or a Pick). Confirmed: the real array also carries adminOnly, so this is already a partial, hand-copied duplicate of the shape — the kind of thing that silently drifts when a field is added.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, N+1 patterns). This is a small, static nav restructuring — no data fetching, loops, or growth-prone state were touched.
visibleSections = sections.filter(...) (AppShell.tsx:38) re-filters a fixed 3-item array on every render — negligible cost, not worth memoizing.
useMatchRoute() (AppShell.tsx:31,35) adds one route-match check per render; this is the standard TanStack Router API for this purpose and not a new subscription pattern beyond what routing already does.
AccountMenu and BottomNav are declared as top-level functions (not inline closures inside AppShell), so they don't remount/reset state every render — confirmed by reading the file (AppShell.tsx:100, 172).
The backdrop-click-away pattern (AppShell.tsx:141-146) only mounts the extra fixed inset-0 button while the popover is open, not unconditionally — no persistent listener or extra paint cost when closed.
No new network calls, no loops over unbounded collections, no work added to the render hot path beyond trivial array operations on a 3-element list.
Nothing here rises to a reportable performance regression.
🧯 Error handling & edge cases — Minor issues
Both findings check out against the actual code. No toast is fired anywhere on logout failure, useLogout has no onError, and the retry affordance lives exclusively inside the {open && ...} block that setOpen(false) unmounts. The route-persistence claim for finding 2 is also confirmed — AppShell is the root layout component (router.tsx:27), so it doesn't remount across navigations, and the backdrop (z-30) sits above both the top bar and bottom nav (z-20).
VERDICT: Minor issues
Findings (🧯 error handling & edge cases lens):
web/src/components/layout/AppShell.tsx:113 — On logout failure, onLogout's catch block calls setOpen(false), closing the account popover. That popover is the only place the failure is surfaced (title="Sign out failed — try again" and the "Retry sign out" label live inside it, at lines 159/162), and the comment directly above the call still says "leave the user where they are (the button re-enables for a retry)" — the code no longer does that; it hides the retry button instead. useLogout (web/src/lib/auth.ts:86-97) has no onError handler, and no toast fires for this path anywhere in AppShell.tsx or auth.ts, unlike web/src/lib/objects.ts, where every mutation calls toast.error(...) on failure — the codebase's established pattern for surfacing async failures. So on a failed sign-out, the menu silently closes with zero visible feedback; the user has to guess something went wrong and manually reopen the menu to see the "Retry sign out" state. The diff's removed pre-PR onLogout had no setOpen call and the retry button (then inline, not in a popover) was always visible, so this is a regression introduced by moving to the popover. Fix: don't close the popover on failure (drop the setOpen(false) in the catch, or replace it with a toast.error so failure is visible even if the menu later closes).
web/src/components/layout/AppShell.tsx:103 (AccountMenu) — open state is never reset on route change. AppShell is the root route layout component (web/src/router.tsx:27), so it doesn't remount on navigation — only the routed Outlet content changes. The full-screen click-away backdrop (fixed inset-0 z-30, line 144) sits above the bottom nav and top nav links (both z-20), so while the menu is open, a link tap hits the backdrop and closes the menu instead of navigating. Browser back/forward, however, changes the route directly without going through that click, so the backdrop and menu can remain open (and mounted) over a newly-navigated page, swallowing the next tap. Minor/edge-case, but it's a missing-cleanup gap: nothing ties open to the route. A useEffect resetting open on location change (or closing on Link click as well as backdrop click) would close the gap.
Authz (Settings admin gate) is preserved.visibleSections filters Settings by user?.isAdmin (AppShell.tsx:38), matching the prior admin-only render. Confirmed the real boundary is server-side: router.tsx:130 wires /settingsbeforeLoad to requireAdmin, which checks me.isAdmin (router.tsx:56-61). The PR description and retained comment correctly frame the hidden link as convenience, not the boundary — no regression and no new client-trust issue.
No injection/XSS surface.displayName (zod-validated z.string() in auth.ts:11) is only rendered as React text children (button label, popover "Signed in as …", and the initial derived via charAt(0).toUpperCase()). No dangerouslySetInnerHTML, no template interpolation into attributes/markup.
No credential/secret leakage. Only the user's own displayName is surfaced (and it was already shown sm:inline before); no tokens, emails-in-headers, or other sensitive fields added.
Logout flow integrity unchanged.AccountMenu.onLogout keeps the same fail-leave-in-place semantics (mutate, then navigate to /login only on success), and useLogout's onSuccess (auth.ts:90-95) nulls the me cache and drops non-auth query data. The error branch closing the popover is a UX detail, not a security one.
Backdrop/popover z-ordering (backdrop z-30 < menu z-40) is sound for click routing; no clickjacking-style interception of the sign-out action.
🎯 Correctness — No material issues found
Verdict: No material issues found
Through the correctness lens I verified the change's logic against the actual checked-out code:
Route matching for editor detection — matchRoute({ to: '/gardens/$gardenId', fuzzy: false }) is correct against the registered route at web/src/router.tsx:108-118 (path: 'gardens/$gardenId'). fuzzy:false + the required $gardenId param means the /gardens list route (router.tsx:101-106, path gardens) does not match, so the bottom bar correctly shows on the list and hides in the editor. Search params (?focus=) don't affect route matching, so editor links with focus still match. (Verified useMatchRoute is a standard TanStack Router hook; the dependency is @tanstack/react-router 1.170.18 per web/package-lock.json:1538.)
Admin filtering — visibleSections = sections.filter((s) => !s.adminOnly || user?.isAdmin) (AppShell.tsx:38) reproduces the prior behavior: Gardens/Plants for everyone, Settings only when user.isAdmin, consistent with the requireAdmin guard at router.tsx:56-64 and the isAdmin field in the user schema (lib/auth.ts:12). The as const array is passed into BottomNav's wider ReadonlyArray<{to,label,icon}> param via a variable, so no excess-property check fires — fine.
Logout flow — AccountMenu.onLogout mirrors the old onLogout (await logout.mutateAsync(), then navigate to /login); on error it closes the popover and leaves the session intact. useLogout (lib/auth.ts:86-97) nulls the me cache in onSuccess, which flips user to null and hides the bottom nav / swaps to the login link. Correct.
Z-index/layering of the popover — backdrop fixed inset-0 z-30, menu absolute right-0 z-40: the menu sits above the backdrop so the Sign out button is clickable; the backdrop closes on outside tap. Correct.
Bottom-bar clearance math (re-derived): bar = min-h-[3.25rem] (52px) + py-1.5 (12px) + pb-[env(safe-area-inset-bottom)] ≈ 64px + safe-area; main adds pb-[calc(4.5rem+env(safe-area-inset-bottom))] = 72px + safe-area → ~8px clearance plus the safe area. Content is not hidden. Correct.
No semantic, formula, threshold, or logic defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/components/layout/AppShell.tsx:187 — BottomNav puts text-muted in the link's base className and in inactiveProps (:189). This contradicts the file's own documented convention at lines 14–16 ("state-specific and conflicting utilities (text-muted vs text-fg) live in the state props — never in the base — to avoid ambiguous overrides"). The active state uses text-accent-strong (:188), which the base text-muted will fight under concatenation; the desktop links (:56 + :57-58) follow the convention correctly by keeping color out of navLinkBase. The bottom bar should match: drop text-muted from the base so only the state props carry color.
web/src/components/layout/AppShell.tsx:174-176 — BottomNav's sections prop is typed as a hand-written ReadonlyArray<{ to: string; label: string; icon: string }>, a partial re-declaration of the sections array's shape (:8-12, declared as const with an adminOnly field this type omits). It can silently drift if a field is added or renamed. Prefer deriving the element type (e.g. (typeof sections)[number] and picking the needed fields, or accepting the full objects) so the prop type stays in sync with the source of truth.
Both verified by reading AppShell.tsx in full and comparing the desktop-link path (:52-62) against the bottom-bar path (:183-195).
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens this change is clean. Specifics I verified by reading AppShell.tsx:
visibleSections = sections.filter(...) (line 38) runs every render, but sections is a 3-element as const array — trivial, not a hot path, and strictly cheaper than the old code which rendered the links plus a separate conditional Settings block.
useMatchRoute (line 31) and the inEditor check (line 35) are constant-time hook calls per render; no route-table scan leaks into render work.
BottomNav and AccountMenu are tiny presentational components; .map over ≤3 items, no allocations worth flagging.
No new N+1 behavior, no unbounded growth, no blocking calls, no quadratic patterns. The fixed bottom bar and pb-[calc(...)] padding are static CSS, not runtime work.
One non-blocking note (minor, not material): the new BottomNav (line 180) adds a second always-on backdrop-blur surface (the header at line 42 is the first). Two fixed/sticky backdrop-blur layers re-sampling the backdrop during scroll can cause scroll repaint cost on low-end mobile. If scroll jank shows up in profiling later, drop the bottom bar to a solid bg-surface (no blur); the header blur is the more valuable one. Not worth changing preemptively.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/components/layout/AppShell.tsx:113 — On logout failure the catch calls setOpen(false), closing the popover and hiding the only place the error/retry affordance lives (logout.isError → "Retry sign out" at line 162). The header account-toggle button (lines 121-135) carries no error state, so after a failed sign-out the user gets zero feedback and the retry button is now buried behind reopening the menu. The original code left the button visible so logout.isError could surface. The comment at line 112 even says "the button re-enables for a retry," but closing the popover contradicts that. Fix: don't setOpen(false) on error (keep the popover open so the "Retry sign out" label is visible), or surface the error on the toggle button itself (e.g. a title/aria-label + visual cue driven by logout.isError) before closing.
Other edge cases check out: initial (line 117) safely falls back to '·' for empty/whitespace displayName; visibleSections filter (line 38) handles user being undefined correctly; showBottomNav is false when !user, so BottomNav isn't rendered with an empty list; the popover closes cleanly on outside tap via the z-30 backdrop (lines 141-146) covering the z-20 header.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
This PR is purely a UI/layout refactor — moving navigation from a desktop-oriented top bar to a mobile-first bottom tab bar + slim top bar. Through the security lens, there's nothing material to report:
No injection surfaces: All route targets (/gardens, /plants, /settings, /gardens/$gardenId, /login) are hardcoded constants. The sections array and all Linkto props are static strings; no user input reaches a URL, path, command, template, or query.
No XSS via displayName:user.displayName is rendered only as JSX text content (lines 128, 152 of AppShell.tsx), which React escapes automatically. It never reaches a dangerouslySetInnerHTML, href, src, CSS className, or JavaScript context. The computed initial is produced via charAt(0).toUpperCase() — safe.
AuthZ is unchanged: The Settings link is still gated client-side by user?.isAdmin, and the PR explicitly (and correctly) notes the server is the security boundary. No authn/authz gap introduced.
No open-redirect or SSRF: Navigation (useNavigate) is to the hardcoded /login path only.
Popover backdrop is benign: The fixed-position click-away <button> (z-30) is a standard, safe pattern that avoids a persistent document listener; it doesn't create a clickjacking vector.
🎯 Correctness — Blocking issues found
Blocking issues found
web/src/components/layout/AppShell.tsx:187 — The BottomNavLink base className includes text-muted, but activeProps only adds text-accent-strong. The file's own comment (line 14-16) explains that TanStack Router concatenates base + state classes, so conflicting color utilities must live only in activeProps/inactiveProps to avoid ambiguous overrides. Because text-muted is present in the base string, both classes end up on the active tab element; whichever Tailwind emits later in the CSS wins. If .text-muted happens to follow .text-accent-strong in the generated stylesheet (determined by scan order, not source order), the active tab will stay muted instead of highlighted. Fix: Remove text-muted from the base className and add it to inactiveProps.className.
web/src/components/layout/AppShell.tsx:113 — In the onLogout catch handler, setOpen(false) closes the account popover when logout fails. The comment two lines above explicitly says the intent is to "leave the user where they are (the button re-enables for a retry)." Closing the menu hides the button entirely, so the user cannot retry without first reopening the menu; the error state (logout.isError → "Retry sign out") is no longer visible. Fix: Remove setOpen(false) from the catch block so the menu stays open and the retry button remains accessible.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/layout/AppShell.tsx:84 — The <main> element uses a template-string className for conditional padding instead of the project's cn() utility. The codebase consistently uses cn() for conditional class composition (e.g., web/src/components/ui/Button.tsx, web/src/editor/ChatPanel.tsx). This should be className={cn('mx-auto w-full max-w-5xl flex-1 px-4 py-6', showBottomNav && 'pb-[calc(4.5rem+env(safe-area-inset-bottom))] md:pb-6')} to stay consistent and avoid Tailwind conflict ambiguity.
web/src/components/layout/AppShell.tsx:187 — BottomNav puts text-muted in the base className while also repeating it in inactiveProps. The comment at lines 14–19 explicitly documents the project convention that state-specific utilities belong only in activeProps/inactiveProps to avoid ambiguous overrides when TanStack Router concatenates classes. The base should drop text-muted (and any other color/state utilities) so the inactive state owns it, matching the pattern used for the desktop nav links.
⚡ Performance — No material issues found
No material issues found.
AppShell.tsx:35 — matchRoute is called on every render, but useMatchRoute returns a stable function and the match check itself is cheap; this is idiomatic TanStack Router usage and not a hot path concern.
AppShell.tsx:38 — visibleSections is re-filtered on every render, but the source array has at most 3 elements; negligible cost.
No unbounded growth, N+1 queries, unnecessary allocations in hot loops, or blocking calls introduced by this change.
🧯 Error handling & edge cases — Minor issues
Minor issues
AppShell.tsx:113 — Account menu closes on logout error, hiding retry feedback:
In AccountMenu.onLogout, the catch block calls setOpen(false) when logout.mutateAsync() fails. Because logout.isError drives the button label ("Retry sign out"), closing the menu immediately means the user never sees that the request failed or the retry prompt. They must reopen the menu to discover the error state. Fix: Remove setOpen(false) from the catch block so the menu stays open and the error state remains visible for an immediate retry. (If logout succeeds but navigate throws, the component will unmount via the useLogout.onSuccess cache clear anyway.)
Settings route gating is consistent with the server: the bottom bar's adminOnly filter (user?.isAdmin) only hides the link as a convenience, while the API still enforces requireAdmin (internal/api/api.go:158, internal/service/instance_settings.go:21) and returns 403 for non-admins — exactly the security posture the prior inline link documented, preserved here.
The account popover renders displayName as text content (initial via displayName.trim().charAt(0)), with no dangerouslySetInnerHTML or HTML interpolation anywhere in the diff, so no XSS surface is introduced through the new UI.
Logout/error handling is unchanged in behavior (still calls logout.mutateAsync, leaves the user in place on failure, navigates to /login on success); the popover merely relocates the control.
The click-away backdrop is a sibling <button> with z-30 over the menu (z-40) ordering — purely a UI concern, not a security one.
I verified the /settings server gate at internal/api/api.go:158 and the requireAdmin service check at internal/service/instance_settings.go:21-34, and confirmed the repo has no dangerouslySetInnerHTML usage in web/src.
🎯 Correctness — Minor issues
The finding is confirmed against the actual code:
AppShell.tsx:35-36 — inEditor only matches /gardens/$gardenId; /g/$token is not excluded, so showBottomNav is true for any signed-in viewer on the public share route.
router.tsx:137-141 — publicGardenRoute is g/$token, a separate full-screen canvas route with no auth guard (so logged-in users can reach it).
PublicGardenPage.tsx:56 — the container uses h-[calc(100dvh-8rem)], a fixed viewport-relative height that only accounts for the top header. The bottom bar (BottomNav, fixed inset-x-0 bottom-0 z-20, ~52px via min-h-[3.25rem] plus safe-area padding) overlays the bottom of that canvas.
Correctness review
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:35 — bottom bar overlaps the public-garden canvas for logged-in viewers.inEditor only matches /gardens/$gardenId, so the bottom bar is correctly hidden in the editor — but the other full-screen canvas route, /g/$token (publicGardenRoute, web/src/router.tsx:137), is not excluded. showBottomNav = !!user && !inEditor is true for any signed-in visitor viewing a share link on mobile. PublicGardenPage sizes its container with h-[calc(100dvh-8rem)] (web/src/pages/PublicGardenPage.tsx:56), which only subtracts the top header; the new ~52px+safe-area bottom bar then sits on top of the canvas bottom, obscuring it. Before this PR there was no bottom bar anywhere, so that calc was correct; this change regresses it for the logged-in-on-a-shared-link case. Fix: extend the hide condition to the public route, e.g. const inFullScreen = !!matchRoute({ to: '/gardens/$gardenId', fuzzy: false }) || !!matchRoute({ to: '/g/$token', fuzzy: false }) and key showBottomNav off that.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:187 — The BottomNav Link's base className includes text-muted, which both duplicates inactiveProps (line 189: text-muted hover:text-fg) and conflicts with activeProps (line 188: text-accent-strong) when active. TanStack concatenates base + state classes, so an active tab ends up with both text-muted and text-accent-strong — order-dependent and ambiguous. This directly contradicts the in-file convention comment at lines 14–16 ("state-specific and conflicting utilities … live in the state props — never in the base") and the desktop links above, whose navLinkBase (line 17) deliberately carries no color for exactly this reason. Fix: drop text-muted from the base on line 187 (it's already in inactiveProps), matching the desktop links' pattern.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens. This is a small presentational shell change; nothing here introduces hot-path work, unbounded growth, N+1 queries, or avoidable quadratic behavior.
Things I checked in web/src/components/layout/AppShell.tsx:
sections.filter(...) at line 38 and the .map renders (lines 52, 183) operate on a 3-element constant array — trivial, not hot.
useMatchRoute (line 31) and useMe (line 29) are called unconditionally on every render as is correct for hooks; their cost is negligible and unchanged in character from the prior useNavigate/useLogout calls.
The full-screen fixed backdrop <button> (line 141) only mounts while the account popover is open; it's not on any persistent path and adds no continuous cost.
The fixed backdrop-blur bottom bar (line 180) does force a compositing layer and can raise repaint cost during scroll on mobile, but the existing sticky top bar already uses backdrop-blur (line 42), so this doesn't introduce a new class of cost — and it's md:hidden, so desktop is unaffected. Not material enough to flag as a regression.
No performance findings to report.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:109-114 — logout-failure retry affordance is hidden. In AccountMenu.onLogout's catch block, setOpen(false) (line 113) dismisses the popover on error. The "Retry sign out" label and the disabled-state affordance both live inside that popover — the popover only renders when open is true (line 137), and the button whose label flips to "Retry sign out" via logout.isError is at lines 154-163. So after a transient logout failure the menu is dismissed and the user sees no indication anything went wrong or that a retry is available; they must happen to reopen the menu to discover the retry button. The comment at lines 110-112 ("the button re-enables for a retry") describes the old inline-button behavior that the diff removed — the button no longer stays in place, so the comment is now stale and the behavior it claims is gone. This is a real unhappy-path regression: a transient logout failure now looks like nothing happened. Verified against useLogout in auth.ts:86-97 (the mutation's isError/isPending state drives the label, but only renders while open). Suggested fix: drop setOpen(false) from the catch so the menu stays open and the "Retry sign out" button remains visible; or surface the failure via a toast and leave the menu open. Update the stale comment either way.
The remaining unhappy paths (pending logout with menu close via the backdrop, mutation resolving after popover unmount on success) are benign — TanStack Query mutations and useNavigate tolerate component unmount, and showBottomNav/visibleSections are only rendered when user is non-null, so the null-user branch can't reach BottomNav. The displayName.trim().charAt(0).toUpperCase() || '·' fallback at line 117 correctly handles empty/whitespace-only names (the || catches the empty string from charAt(0)), matching userSchema in auth.ts:8-16 where displayName: z.string() permits empty strings. Nothing else material in this lens.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
This PR is a pure UI/layout refactor. From the Security lens:
No authn/authz boundary moved: The user?.isAdmin client-side filter for the Settings link is explicitly documented as UI convenience; the server's requireAdmin gate remains the actual security boundary. Nothing here weakens that.
No injection surface: All Linkto props are hardcoded constants; no user-controlled URLs or path injection.
No XSS via displayName:displayName is rendered in standard JSX (<span>{displayName}</span>), which React escapes. The initial derivation (charAt(0).toUpperCase()) operates on a z.string()-validated field and is likewise escaped.
No credential leakage: The popover renders the user's own display name and a logout button — no secrets exposed.
No unsafe deserialization or SSRF: No data parsing or outbound requests introduced.
The useMatchRoute call is used solely for chrome layout (hiding the bottom bar in the editor), not for any permission decision. Logout handling is unchanged from the prior implementation.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/layout/AppShell.tsx:84-86 — The <main> element uses a template-literal className for conditional padding instead of the project's cn() utility (confirmed via grep that cn() is used in ~27 other components for exactly this pattern). Template literals force readers to parse string concatenation and risk Tailwind class-scanning misses; cn('mx-auto w-full max-w-5xl flex-1 px-4 py-6', showBottomNav && 'pb-[calc(4.5rem+env(safe-area-inset-bottom))] md:pb-6') is consistent with the rest of the codebase and easier to maintain.
⚡ Performance — Minor issues
Minor issues
web/src/components/layout/AppShell.tsx:180 — The mobile bottom nav adds backdrop-blur alongside the header's existing blur, creating two simultaneous expensive compositor layers on mobile. During scroll, the browser must continuously capture and blur content behind both fixed elements, which can cause frame drops and increased battery usage on lower-end devices. The visual benefit is marginal because bg-surface/95 is already nearly opaque. Fix: Remove backdrop-blur from the bottom nav (or switch to an opaque bg-surface background).
🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling & edge-cases lens, the PR is clean:
displayName edge cases are handled.displayName.trim().charAt(0).toUpperCase() || '·' correctly falls back to · for empty or whitespace-only strings.
matchRoute failure modes are safe.!!matchRoute(...) coerces any falsy return to false; the route string is statically typed by TanStack Router.
Logout error handling is preserved. The try/catch around mutateAsync + navigate behaves identically to the old inline code; setOpen(false) in the catch is benign.
No nil/null dereferences introduced.user?.isAdmin is used safely; visibleSections is only rendered when user is truthy.
No missing cleanup or rollback. The popover is pure React state with a backdrop button; no useEffect or document listener leaks.
Nothing in my area.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 7 findings (5 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | Logout failure closes the popover, hiding the only retry/error affordance with no toast fallback | `web/src/components/layout/AppShell.tsx:113` | 4/5 | correctness, error-handling, maintainability |
| 🔴 | BottomNav active tab has conflicting text-muted/text-accent-strong classes | `web/src/components/layout/AppShell.tsx:187` | 3/5 | correctness, maintainability |
| 🟡 | Template-string className instead of cn() utility | `web/src/components/layout/AppShell.tsx:84` | 3/5 | maintainability |
| 🟠 | Bottom-nav suppression misses g/$token, overlapping PublicGardenPage's hardcoded 100dvh-8rem canvas height for signed-in viewers | `web/src/components/layout/AppShell.tsx:35` | 2/5 | correctness |
| 🟡 | BottomNav sections prop type is a hand-written partial copy of the sections array shape, prone to drift | `web/src/components/layout/AppShell.tsx:175` | 2/5 | maintainability |
<details><summary>2 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | Account popover 'open' state isn't reset on route change, leaving an invisible full-screen backdrop after back/forward navigation | `web/src/components/layout/AppShell.tsx:103` | claude-code/sonnet | error-handling |
| 🟡 | Second backdrop-blur layer adds mobile scroll/compositor cost | `web/src/components/layout/AppShell.tsx:180` | opencode/kimi-k2.6:cloud | performance |
</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>
Server-side enforcement is intact — this is purely a client-side nav/UI change over React, with text content auto-escaped (no `dangerouslySetInnerHTML`), no new API calls, no new user-controlled data flowing into URLs, storage, or templates.
VERDICT: No material issues found
- The admin-only filtering (`AppShell.tsx:38`) is UI convenience only; verified the server still gates `/settings` behind `requireAuth()` + `requireAdmin()` in `internal/api/api.go:158` and `internal/api/settings.go:22`, matching the pre-existing pattern the removed comment described. No authz regression.
- `displayName` is rendered as plain JSX text (`AppShell.tsx:128,152`), not via `dangerouslySetInnerHTML` or a URL/attribute sink — React escapes it, so no XSS vector even though it's user-controlled.
- The click-away backdrop (`AppShell.tsx:141-146`) and popover use local component state only; no new storage, no new endpoints, no tokens or secrets touched.
- Route matching (`useMatchRoute`) and section list are static, hard-coded arrays — no injection surface.
Nothing here changes trust boundaries, request handling, or credential flow — it's a pure client-side layout/nav refactor.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings confirmed against the actual code. `publicGardenRoute` (`g/$token`) is a child of `rootRoute` whose component is `AppShell`, has no auth guard, and `PublicGardenPage.tsx:56` hardcodes the same `h-[calc(100dvh-8rem)]` as the editor — but `AppShell.tsx:35` only exempts `gardens/$gardenId` from `showBottomNav`, so a signed-in user on their own share link gets the overlay. The `onLogout` catch block does call `setOpen(false)` at line 113, closing the popover that shows the retry state described in the adjacent comment.
VERDICT: Minor issues
- `web/src/components/layout/AppShell.tsx:35` — Bottom-nav suppression (`inEditor`) only matches `gardens/$gardenId`; it doesn't exempt `g/$token`, which is also a full-screen canvas route (`router.tsx:137-141`, nested under `AppShell` via `rootRoute`, no auth guard so a signed-in user keeps their session there). `PublicGardenPage.tsx:56` hardcodes `h-[calc(100dvh-8rem)]`, the same calc `GardenEditorPage.tsx:405` uses but is correctly exempted for via `inEditor`. For a signed-in user on their own share link, `showBottomNav` becomes `true`, adding `pb-[calc(4.5rem+env(safe-area-inset-bottom))]` to `main` and a `fixed` `BottomNav` (z-20) that the page's fixed-height canvas doesn't leave room for — the bottom of the pan/zoom canvas ends up under the tab bar. Fix: fold `g/$token` into the full-screen check, or make the page's height calc account for the bottom bar.
- `web/src/components/layout/AppShell.tsx:113` — `onLogout`'s catch branch calls `setOpen(false)`, closing the account popover on a failed sign-out. The comment directly above says the intent is to leave the user in place so "the button re-enables for a retry," but closing the popover hides the "Retry sign out" label (driven by `logout.isError`, rendered at line 162) until the user reopens the menu — an extra tap contrary to the comment's stated intent. Fix: drop `setOpen(false)` in the catch branch.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `web/src/components/layout/AppShell.tsx:113` — In `AccountMenu`'s `onLogout`, the catch-block comment ("leave the user where they are ... rather than pretending they're signed out") is directly contradicted by the `setOpen(false)` on the next line: a failed sign-out closes the account popover, hiding the `logout.isError`-driven "Retry sign out" state the comment describes. The user has to reopen the menu to retry. Either the behavior should not close the popover on error, or the comment needs to be rewritten to describe why closing is still fine — as written it's stale/misleading, which CLAUDE.md flags as worse than no comment. Verified by reading the full function (lines 105-115).
- `web/src/components/layout/AppShell.tsx:85` (clearance) vs. `:180,187` (bar height) — `<main>`'s bottom padding is a hardcoded `pb-[calc(4.5rem+env(safe-area-inset-bottom))]`, while the actual `BottomNav` height is built from separate, independently-chosen values (`min-h-[3.25rem]` per link, `py-1.5`, `border-t`, plus its own `pb-[env(safe-area-inset-bottom)]`). Nothing ties these two together — confirmed by reading both blocks. If `BottomNav`'s height changes later, it's easy to forget the matching `pb-` update in `AppShell`, silently reintroducing the "content hidden behind the bar" bug this PR fixes. Worth a shared constant or a cross-referencing comment.
- `web/src/components/layout/AppShell.tsx:175` — `BottomNav`'s `sections` prop is typed by hand as `ReadonlyArray<{ to: string; label: string; icon: string }>` rather than derived from the actual source of truth, the `sections` array at lines 8-12 (`typeof sections[number]` or a `Pick`). Confirmed: the real array also carries `adminOnly`, so this is already a partial, hand-copied duplicate of the shape — the kind of thing that silently drifts when a field is added.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, N+1 patterns). This is a small, static nav restructuring — no data fetching, loops, or growth-prone state were touched.
- `visibleSections = sections.filter(...)` (`AppShell.tsx:38`) re-filters a fixed 3-item array on every render — negligible cost, not worth memoizing.
- `useMatchRoute()` (`AppShell.tsx:31,35`) adds one route-match check per render; this is the standard TanStack Router API for this purpose and not a new subscription pattern beyond what routing already does.
- `AccountMenu` and `BottomNav` are declared as top-level functions (not inline closures inside `AppShell`), so they don't remount/reset state every render — confirmed by reading the file (`AppShell.tsx:100`, `172`).
- The backdrop-click-away pattern (`AppShell.tsx:141-146`) only mounts the extra `fixed inset-0` button while the popover is `open`, not unconditionally — no persistent listener or extra paint cost when closed.
- No new network calls, no loops over unbounded collections, no work added to the render hot path beyond trivial array operations on a 3-element list.
Nothing here rises to a reportable performance regression.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings check out against the actual code. No toast is fired anywhere on logout failure, `useLogout` has no `onError`, and the retry affordance lives exclusively inside the `{open && ...}` block that `setOpen(false)` unmounts. The route-persistence claim for finding 2 is also confirmed — `AppShell` is the root layout component (`router.tsx:27`), so it doesn't remount across navigations, and the backdrop (`z-30`) sits above both the top bar and bottom nav (`z-20`).
## VERDICT: Minor issues
**Findings (🧯 error handling & edge cases lens):**
- **`web/src/components/layout/AppShell.tsx:113`** — On logout failure, `onLogout`'s catch block calls `setOpen(false)`, closing the account popover. That popover is the only place the failure is surfaced (`title="Sign out failed — try again"` and the "Retry sign out" label live inside it, at lines 159/162), and the comment directly above the call still says "leave the user where they are (the button re-enables for a retry)" — the code no longer does that; it hides the retry button instead. `useLogout` (`web/src/lib/auth.ts:86-97`) has no `onError` handler, and no toast fires for this path anywhere in `AppShell.tsx` or `auth.ts`, unlike `web/src/lib/objects.ts`, where every mutation calls `toast.error(...)` on failure — the codebase's established pattern for surfacing async failures. So on a failed sign-out, the menu silently closes with zero visible feedback; the user has to guess something went wrong and manually reopen the menu to see the "Retry sign out" state. The diff's removed pre-PR `onLogout` had no `setOpen` call and the retry button (then inline, not in a popover) was always visible, so this is a regression introduced by moving to the popover. Fix: don't close the popover on failure (drop the `setOpen(false)` in the catch, or replace it with a `toast.error` so failure is visible even if the menu later closes).
- **`web/src/components/layout/AppShell.tsx:103`** (`AccountMenu`) — `open` state is never reset on route change. `AppShell` is the root route layout component (`web/src/router.tsx:27`), so it doesn't remount on navigation — only the routed `Outlet` content changes. The full-screen click-away backdrop (`fixed inset-0 z-30`, line 144) sits above the bottom nav and top nav links (both `z-20`), so while the menu is open, a link tap hits the backdrop and closes the menu instead of navigating. Browser back/forward, however, changes the route directly without going through that click, so the backdrop and menu can remain open (and mounted) over a newly-navigated page, swallowing the next tap. Minor/edge-case, but it's a missing-cleanup gap: nothing ties `open` to the route. A `useEffect` resetting `open` on location change (or closing on `Link` click as well as backdrop click) would close the gap.
</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**
Through the security lens, this change is clean:
- **Authz (Settings admin gate) is preserved.** `visibleSections` filters Settings by `user?.isAdmin` (AppShell.tsx:38), matching the prior admin-only render. Confirmed the real boundary is server-side: `router.tsx:130` wires `/settings` `beforeLoad` to `requireAdmin`, which checks `me.isAdmin` (router.tsx:56-61). The PR description and retained comment correctly frame the hidden link as convenience, not the boundary — no regression and no new client-trust issue.
- **No injection/XSS surface.** `displayName` (zod-validated `z.string()` in auth.ts:11) is only rendered as React text children (button label, popover "Signed in as …", and the `initial` derived via `charAt(0).toUpperCase()`). No `dangerouslySetInnerHTML`, no template interpolation into attributes/markup.
- **No credential/secret leakage.** Only the user's own `displayName` is surfaced (and it was already shown `sm:inline` before); no tokens, emails-in-headers, or other sensitive fields added.
- **Logout flow integrity unchanged.** `AccountMenu.onLogout` keeps the same fail-leave-in-place semantics (mutate, then navigate to `/login` only on success), and `useLogout`'s `onSuccess` (auth.ts:90-95) nulls the `me` cache and drops non-auth query data. The error branch closing the popover is a UX detail, not a security one.
- **Backdrop/popover z-ordering** (`backdrop z-30` < `menu z-40`) is sound for click routing; no clickjacking-style interception of the sign-out action.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the correctness lens I verified the change's logic against the actual checked-out code:
- **Route matching for editor detection** — `matchRoute({ to: '/gardens/$gardenId', fuzzy: false })` is correct against the registered route at `web/src/router.tsx:108-118` (`path: 'gardens/$gardenId'`). `fuzzy:false` + the required `$gardenId` param means the `/gardens` list route (`router.tsx:101-106`, path `gardens`) does *not* match, so the bottom bar correctly shows on the list and hides in the editor. Search params (`?focus=`) don't affect route matching, so editor links with focus still match. (Verified `useMatchRoute` is a standard TanStack Router hook; the dependency is `@tanstack/react-router` 1.170.18 per `web/package-lock.json:1538`.)
- **Admin filtering** — `visibleSections = sections.filter((s) => !s.adminOnly || user?.isAdmin)` (`AppShell.tsx:38`) reproduces the prior behavior: Gardens/Plants for everyone, Settings only when `user.isAdmin`, consistent with the `requireAdmin` guard at `router.tsx:56-64` and the `isAdmin` field in the user schema (`lib/auth.ts:12`). The `as const` array is passed into `BottomNav`'s wider `ReadonlyArray<{to,label,icon}>` param via a variable, so no excess-property check fires — fine.
- **Logout flow** — `AccountMenu.onLogout` mirrors the old `onLogout` (await `logout.mutateAsync()`, then navigate to `/login`); on error it closes the popover and leaves the session intact. `useLogout` (`lib/auth.ts:86-97`) nulls the `me` cache in `onSuccess`, which flips `user` to null and hides the bottom nav / swaps to the login link. Correct.
- **Z-index/layering of the popover** — backdrop `fixed inset-0 z-30`, menu `absolute right-0 z-40`: the menu sits above the backdrop so the Sign out button is clickable; the backdrop closes on outside tap. Correct.
- **Bottom-bar clearance math** (re-derived): bar = `min-h-[3.25rem]` (52px) + `py-1.5` (12px) + `pb-[env(safe-area-inset-bottom)]` ≈ 64px + safe-area; main adds `pb-[calc(4.5rem+env(safe-area-inset-bottom))]` = 72px + safe-area → ~8px clearance plus the safe area. Content is not hidden. Correct.
No semantic, formula, threshold, or logic defects found in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Verdict: Minor issues
- `web/src/components/layout/AppShell.tsx:187` — `BottomNav` puts `text-muted` in the link's base className *and* in `inactiveProps` (`:189`). This contradicts the file's own documented convention at lines 14–16 ("state-specific and conflicting utilities (text-muted vs text-fg) live in the state props — never in the base — to avoid ambiguous overrides"). The active state uses `text-accent-strong` (`:188`), which the base `text-muted` will fight under concatenation; the desktop links (`:56` + `:57-58`) follow the convention correctly by keeping color out of `navLinkBase`. The bottom bar should match: drop `text-muted` from the base so only the state props carry color.
- `web/src/components/layout/AppShell.tsx:174-176` — `BottomNav`'s `sections` prop is typed as a hand-written `ReadonlyArray<{ to: string; label: string; icon: string }>`, a partial re-declaration of the `sections` array's shape (`:8-12`, declared `as const` with an `adminOnly` field this type omits). It can silently drift if a field is added or renamed. Prefer deriving the element type (e.g. `(typeof sections)[number]` and picking the needed fields, or accepting the full objects) so the prop type stays in sync with the source of truth.
Both verified by reading `AppShell.tsx` in full and comparing the desktop-link path (`:52-62`) against the bottom-bar path (`:183-195`).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the performance lens this change is clean. Specifics I verified by reading `AppShell.tsx`:
- `visibleSections = sections.filter(...)` (line 38) runs every render, but `sections` is a 3-element `as const` array — trivial, not a hot path, and strictly cheaper than the old code which rendered the links plus a separate conditional Settings block.
- `useMatchRoute` (line 31) and the `inEditor` check (line 35) are constant-time hook calls per render; no route-table scan leaks into render work.
- `BottomNav` and `AccountMenu` are tiny presentational components; `.map` over ≤3 items, no allocations worth flagging.
- No new N+1 behavior, no unbounded growth, no blocking calls, no quadratic patterns. The fixed bottom bar and `pb-[calc(...)]` padding are static CSS, not runtime work.
One non-blocking note (minor, not material): the new `BottomNav` (line 180) adds a second always-on `backdrop-blur` surface (the header at line 42 is the first). Two fixed/sticky `backdrop-blur` layers re-sampling the backdrop during scroll can cause scroll repaint cost on low-end mobile. If scroll jank shows up in profiling later, drop the bottom bar to a solid `bg-surface` (no blur); the header blur is the more valuable one. Not worth changing preemptively.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Verdict: Minor issues
- `web/src/components/layout/AppShell.tsx:113` — On logout failure the `catch` calls `setOpen(false)`, closing the popover and hiding the only place the error/retry affordance lives (`logout.isError` → "Retry sign out" at line 162). The header account-toggle button (lines 121-135) carries no error state, so after a failed sign-out the user gets **zero feedback** and the retry button is now buried behind reopening the menu. The original code left the button visible so `logout.isError` could surface. The comment at line 112 even says "the button re-enables for a retry," but closing the popover contradicts that. Fix: don't `setOpen(false)` on error (keep the popover open so the "Retry sign out" label is visible), or surface the error on the toggle button itself (e.g. a `title`/aria-label + visual cue driven by `logout.isError`) before closing.
Other edge cases check out: `initial` (line 117) safely falls back to `'·'` for empty/whitespace `displayName`; `visibleSections` filter (line 38) handles `user` being undefined correctly; `showBottomNav` is `false` when `!user`, so `BottomNav` isn't rendered with an empty list; the popover closes cleanly on outside tap via the z-30 backdrop (lines 141-146) covering the z-20 header.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
This PR is purely a UI/layout refactor — moving navigation from a desktop-oriented top bar to a mobile-first bottom tab bar + slim top bar. Through the security lens, there's nothing material to report:
- **No injection surfaces:** All route targets (`/gardens`, `/plants`, `/settings`, `/gardens/$gardenId`, `/login`) are hardcoded constants. The `sections` array and all `Link` `to` props are static strings; no user input reaches a URL, path, command, template, or query.
- **No XSS via `displayName`:** `user.displayName` is rendered only as JSX text content (lines 128, 152 of AppShell.tsx), which React escapes automatically. It never reaches a `dangerouslySetInnerHTML`, `href`, `src`, CSS className, or JavaScript context. The computed `initial` is produced via `charAt(0).toUpperCase()` — safe.
- **AuthZ is unchanged:** The Settings link is still gated client-side by `user?.isAdmin`, and the PR explicitly (and correctly) notes the server is the security boundary. No authn/authz gap introduced.
- **No open-redirect or SSRF:** Navigation (`useNavigate`) is to the hardcoded `/login` path only.
- **Popover backdrop is benign:** The fixed-position click-away `<button>` (z-30) is a standard, safe pattern that avoids a persistent document listener; it doesn't create a clickjacking vector.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/components/layout/AppShell.tsx:187`** — The `BottomNav` `Link` base `className` includes `text-muted`, but `activeProps` only adds `text-accent-strong`. The file's own comment (line 14-16) explains that TanStack Router concatenates base + state classes, so conflicting color utilities must live *only* in `activeProps`/`inactiveProps` to avoid ambiguous overrides. Because `text-muted` is present in the base string, both classes end up on the active tab element; whichever Tailwind emits later in the CSS wins. If `.text-muted` happens to follow `.text-accent-strong` in the generated stylesheet (determined by scan order, not source order), the active tab will stay muted instead of highlighted.
**Fix:** Remove `text-muted` from the base `className` and add it to `inactiveProps.className`.
- **`web/src/components/layout/AppShell.tsx:113`** — In the `onLogout` catch handler, `setOpen(false)` closes the account popover when logout fails. The comment two lines above explicitly says the intent is to "leave the user where they are (the button re-enables for a retry)." Closing the menu hides the button entirely, so the user cannot retry without first reopening the menu; the error state (`logout.isError` → "Retry sign out") is no longer visible.
**Fix:** Remove `setOpen(false)` from the catch block so the menu stays open and the retry button remains accessible.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `web/src/components/layout/AppShell.tsx:84` — The `<main>` element uses a template-string `className` for conditional padding instead of the project's `cn()` utility. The codebase consistently uses `cn()` for conditional class composition (e.g., `web/src/components/ui/Button.tsx`, `web/src/editor/ChatPanel.tsx`). This should be `className={cn('mx-auto w-full max-w-5xl flex-1 px-4 py-6', showBottomNav && 'pb-[calc(4.5rem+env(safe-area-inset-bottom))] md:pb-6')}` to stay consistent and avoid Tailwind conflict ambiguity.
- `web/src/components/layout/AppShell.tsx:187` — `BottomNav` puts `text-muted` in the base `className` while also repeating it in `inactiveProps`. The comment at lines 14–19 explicitly documents the project convention that state-specific utilities belong *only* in `activeProps`/`inactiveProps` to avoid ambiguous overrides when TanStack Router concatenates classes. The base should drop `text-muted` (and any other color/state utilities) so the inactive state owns it, matching the pattern used for the desktop nav links.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
- `AppShell.tsx:35` — `matchRoute` is called on every render, but `useMatchRoute` returns a stable function and the match check itself is cheap; this is idiomatic TanStack Router usage and not a hot path concern.
- `AppShell.tsx:38` — `visibleSections` is re-filtered on every render, but the source array has at most 3 elements; negligible cost.
- No unbounded growth, N+1 queries, unnecessary allocations in hot loops, or blocking calls introduced by this change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`AppShell.tsx:113` — Account menu closes on logout error, hiding retry feedback:**
In `AccountMenu.onLogout`, the catch block calls `setOpen(false)` when `logout.mutateAsync()` fails. Because `logout.isError` drives the button label ("Retry sign out"), closing the menu immediately means the user never sees that the request failed or the retry prompt. They must reopen the menu to discover the error state.
**Fix:** Remove `setOpen(false)` from the catch block so the menu stays open and the error state remains visible for an immediate retry. (If logout succeeds but `navigate` throws, the component will unmount via the `useLogout.onSuccess` cache clear anyway.)
</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
- Settings route gating is consistent with the server: the bottom bar's `adminOnly` filter (`user?.isAdmin`) only hides the link as a convenience, while the API still enforces `requireAdmin` (`internal/api/api.go:158`, `internal/service/instance_settings.go:21`) and returns 403 for non-admins — exactly the security posture the prior inline link documented, preserved here.
- The account popover renders `displayName` as text content (initial via `displayName.trim().charAt(0)`), with no `dangerouslySetInnerHTML` or HTML interpolation anywhere in the diff, so no XSS surface is introduced through the new UI.
- Logout/error handling is unchanged in behavior (still calls `logout.mutateAsync`, leaves the user in place on failure, navigates to `/login` on success); the popover merely relocates the control.
- The click-away backdrop is a sibling `<button>` with `z-30` over the menu (`z-40`) ordering — purely a UI concern, not a security one.
I verified the `/settings` server gate at `internal/api/api.go:158` and the `requireAdmin` service check at `internal/service/instance_settings.go:21-34`, and confirmed the repo has no `dangerouslySetInnerHTML` usage in `web/src`.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed against the actual code:
- `AppShell.tsx:35-36` — `inEditor` only matches `/gardens/$gardenId`; `/g/$token` is not excluded, so `showBottomNav` is true for any signed-in viewer on the public share route.
- `router.tsx:137-141` — `publicGardenRoute` is `g/$token`, a separate full-screen canvas route with no auth guard (so logged-in users can reach it).
- `PublicGardenPage.tsx:56` — the container uses `h-[calc(100dvh-8rem)]`, a fixed viewport-relative height that only accounts for the top header. The bottom bar (`BottomNav`, `fixed inset-x-0 bottom-0 z-20`, ~52px via `min-h-[3.25rem]` plus safe-area padding) overlays the bottom of that canvas.
## Correctness review
**VERDICT: Minor issues**
- **`web/src/components/layout/AppShell.tsx:35` — bottom bar overlaps the public-garden canvas for logged-in viewers.** `inEditor` only matches `/gardens/$gardenId`, so the bottom bar is correctly hidden in the editor — but the other full-screen canvas route, `/g/$token` (`publicGardenRoute`, `web/src/router.tsx:137`), is not excluded. `showBottomNav = !!user && !inEditor` is true for any signed-in visitor viewing a share link on mobile. `PublicGardenPage` sizes its container with `h-[calc(100dvh-8rem)]` (`web/src/pages/PublicGardenPage.tsx:56`), which only subtracts the top header; the new ~52px+safe-area bottom bar then sits on top of the canvas bottom, obscuring it. Before this PR there was no bottom bar anywhere, so that calc was correct; this change regresses it for the logged-in-on-a-shared-link case. Fix: extend the hide condition to the public route, e.g. `const inFullScreen = !!matchRoute({ to: '/gardens/$gardenId', fuzzy: false }) || !!matchRoute({ to: '/g/$token', fuzzy: false })` and key `showBottomNav` off that.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**VERDICT: Minor issues**
- **`web/src/components/layout/AppShell.tsx:187`** — The `BottomNav` Link's base className includes `text-muted`, which both **duplicates `inactiveProps`** (line 189: `text-muted hover:text-fg`) and **conflicts with `activeProps`** (line 188: `text-accent-strong`) when active. TanStack concatenates base + state classes, so an active tab ends up with both `text-muted` and `text-accent-strong` — order-dependent and ambiguous. This directly contradicts the in-file convention comment at lines 14–16 ("state-specific and conflicting utilities … live in the state props — never in the base") and the desktop links above, whose `navLinkBase` (line 17) deliberately carries no color for exactly this reason. Fix: drop `text-muted` from the base on line 187 (it's already in `inactiveProps`), matching the desktop links' pattern.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
Reviewed through the performance lens. This is a small presentational shell change; nothing here introduces hot-path work, unbounded growth, N+1 queries, or avoidable quadratic behavior.
Things I checked in `web/src/components/layout/AppShell.tsx`:
- `sections.filter(...)` at line 38 and the `.map` renders (lines 52, 183) operate on a 3-element constant array — trivial, not hot.
- `useMatchRoute` (line 31) and `useMe` (line 29) are called unconditionally on every render as is correct for hooks; their cost is negligible and unchanged in character from the prior `useNavigate`/`useLogout` calls.
- The full-screen fixed backdrop `<button>` (line 141) only mounts while the account popover is `open`; it's not on any persistent path and adds no continuous cost.
- The fixed `backdrop-blur` bottom bar (line 180) does force a compositing layer and can raise repaint cost during scroll on mobile, but the existing sticky top bar already uses `backdrop-blur` (line 42), so this doesn't introduce a new class of cost — and it's `md:hidden`, so desktop is unaffected. Not material enough to flag as a regression.
No performance findings to report.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
## VERDICT: Minor issues
- **`web/src/components/layout/AppShell.tsx:109-114` — logout-failure retry affordance is hidden.** In `AccountMenu.onLogout`'s catch block, `setOpen(false)` (line 113) dismisses the popover on error. The "Retry sign out" label and the disabled-state affordance both live *inside* that popover — the popover only renders when `open` is true (line 137), and the button whose label flips to "Retry sign out" via `logout.isError` is at lines 154-163. So after a transient logout failure the menu is dismissed and the user sees no indication anything went wrong or that a retry is available; they must happen to reopen the menu to discover the retry button. The comment at lines 110-112 ("the button re-enables for a retry") describes the *old* inline-button behavior that the diff removed — the button no longer stays in place, so the comment is now stale and the behavior it claims is gone. This is a real unhappy-path regression: a transient logout failure now looks like nothing happened. Verified against `useLogout` in `auth.ts:86-97` (the mutation's `isError`/`isPending` state drives the label, but only renders while `open`). Suggested fix: drop `setOpen(false)` from the catch so the menu stays open and the "Retry sign out" button remains visible; or surface the failure via a toast and leave the menu open. Update the stale comment either way.
The remaining unhappy paths (pending logout with menu close via the backdrop, mutation resolving after popover unmount on success) are benign — TanStack Query mutations and `useNavigate` tolerate component unmount, and `showBottomNav`/`visibleSections` are only rendered when `user` is non-null, so the null-user branch can't reach `BottomNav`. The `displayName.trim().charAt(0).toUpperCase() || '·'` fallback at line 117 correctly handles empty/whitespace-only names (the `||` catches the empty string from `charAt(0)`), matching `userSchema` in `auth.ts:8-16` where `displayName: z.string()` permits empty strings. Nothing else material in this lens.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
This PR is a pure UI/layout refactor. From the **Security** lens:
- **No authn/authz boundary moved:** The `user?.isAdmin` client-side filter for the Settings link is explicitly documented as UI convenience; the server's `requireAdmin` gate remains the actual security boundary. Nothing here weakens that.
- **No injection surface:** All `Link` `to` props are hardcoded constants; no user-controlled URLs or path injection.
- **No XSS via `displayName`:** `displayName` is rendered in standard JSX (`<span>{displayName}</span>`), which React escapes. The `initial` derivation (`charAt(0).toUpperCase()`) operates on a `z.string()`-validated field and is likewise escaped.
- **No credential leakage:** The popover renders the user's own display name and a logout button — no secrets exposed.
- **No unsafe deserialization or SSRF:** No data parsing or outbound requests introduced.
The `useMatchRoute` call is used solely for chrome layout (hiding the bottom bar in the editor), not for any permission decision. Logout handling is unchanged from the prior implementation.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`web/src/components/layout/AppShell.tsx:84-86`** — The `<main>` element uses a template-literal `className` for conditional padding instead of the project's `cn()` utility (confirmed via grep that `cn()` is used in ~27 other components for exactly this pattern). Template literals force readers to parse string concatenation and risk Tailwind class-scanning misses; `cn('mx-auto w-full max-w-5xl flex-1 px-4 py-6', showBottomNav && 'pb-[calc(4.5rem+env(safe-area-inset-bottom))] md:pb-6')` is consistent with the rest of the codebase and easier to maintain.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Minor issues**
- `web/src/components/layout/AppShell.tsx:180` — The mobile bottom nav adds `backdrop-blur` alongside the header's existing blur, creating **two simultaneous expensive compositor layers** on mobile. During scroll, the browser must continuously capture and blur content behind both fixed elements, which can cause frame drops and increased battery usage on lower-end devices. The visual benefit is marginal because `bg-surface/95` is already nearly opaque.
**Fix:** Remove `backdrop-blur` from the bottom nav (or switch to an opaque `bg-surface` background).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
Through the error-handling & edge-cases lens, the PR is clean:
- **`displayName` edge cases are handled.** `displayName.trim().charAt(0).toUpperCase() || '·'` correctly falls back to `·` for empty or whitespace-only strings.
- **`matchRoute` failure modes are safe.** `!!matchRoute(...)` coerces any falsy return to `false`; the route string is statically typed by TanStack Router.
- **Logout error handling is preserved.** The `try/catch` around `mutateAsync` + `navigate` behaves identically to the old inline code; `setOpen(false)` in the catch is benign.
- **No nil/null dereferences introduced.** `user?.isAdmin` is used safely; `visibleSections` is only rendered when `user` is truthy.
- **No missing cleanup or rollback.** The popover is pure React state with a backdrop button; no `useEffect` or document listener leaks.
Nothing in my area.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve
merged commit 8a27df9e9c into main2026-07-22 05:03:54 +00:00
steve
deleted branch feat/mobile-shell2026-07-22 05:03:54 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #98. Part of #96.
The top bar was desktop-shaped — logo + Gardens/Plants/Settings + name + Sign out all in one row, wrapping "Sign out" to two lines at 390px, with no thumb-reachable navigation.
Mobile-first now
/gardens, which matters in the editor) + a compact account control (right).env(safe-area-inset-bottom)-aware, ≥52px targets, shown only when signed in.md:+) keeps the inline top nav; the bottom bar ismd:hidden./gardens/$gardenId(viauseMatchRoute), so nothing competes there. The editor's own mode bar arrives in #99; until then the brand link is the way back.Verified live
nav[aria-label="Sections"]isn't rendered there) with the brand back-link present.tscclean;vitest87 pass;npm run buildgreen.Screenshots (mobile list w/ bottom bar, account menu, desktop) captured during review.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 05:02:11Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -103,0 +110,4 @@const navigate = useNavigate()const [open, setOpen] = useState(false)const pathname = useRouterState({ select: (s) => s.location.pathname })🟠 Logout failure closes the popover, hiding the only retry/error affordance with no toast fallback
correctness, error-handling, maintainability · flagged by 4 models
web/src/components/layout/AppShell.tsx:113— On logout failure,onLogout's catch block callssetOpen(false), closing the account popover. That popover is the only place the failure is surfaced (title="Sign out failed — try again"and the "Retry sign out" label live inside it, at lines 159/162), and the comment directly above the call still says "leave the user where they are (the button re-enables for a retry)" — the code no longer does that; it hides the retry button instead. `use…🪰 Gadfly · advisory
@@ -103,0 +172,4 @@title={logout.isError ? 'Sign out failed — try again' : undefined}className="w-full rounded-md px-3 py-2 text-left text-sm font-medium text-muted transition-colors hover:bg-border/60 hover:text-fg disabled:opacity-60">{logout.isPending ? 'Signing out…' : logout.isError ? 'Retry sign out' : 'Sign out'}🟡 BottomNav sections prop type is a hand-written partial copy of the sections array shape, prone to drift
maintainability · flagged by 2 models
web/src/components/layout/AppShell.tsx:175—BottomNav'ssectionsprop is typed by hand asReadonlyArray<{ to: string; label: string; icon: string }>rather than derived from the actual source of truth, thesectionsarray at lines 8-12 (typeof sections[number]or aPick). Confirmed: the real array also carriesadminOnly, so this is already a partial, hand-copied duplicate of the shape — the kind of thing that silently drifts when a field is added.🪰 Gadfly · advisory
@@ -103,0 +177,4 @@</div></>)}</div>🟡 Second backdrop-blur layer adds mobile scroll/compositor cost
performance · flagged by 1 model
web/src/components/layout/AppShell.tsx:180— The mobile bottom nav addsbackdrop-bluralongside the header's existing blur, creating two simultaneous expensive compositor layers on mobile. During scroll, the browser must continuously capture and blur content behind both fixed elements, which can cause frame drops and increased battery usage on lower-end devices. The visual benefit is marginal becausebg-surface/95is already nearly opaque. Fix: Removebackdrop-blurfrom the bot…🪰 Gadfly · advisory
@@ -103,0 +184,4 @@/** Mobile bottom tab bar for the top-level sections (thumb zone, safe-area aware). */function BottomNav({ sections }: { sections: ReadonlyArray<Section> }) {return (<nav🔴 BottomNav active tab has conflicting text-muted/text-accent-strong classes
correctness, maintainability · flagged by 3 models
web/src/components/layout/AppShell.tsx:187— TheBottomNavLinkbaseclassNameincludestext-muted, butactivePropsonly addstext-accent-strong. The file's own comment (line 14-16) explains that TanStack Router concatenates base + state classes, so conflicting color utilities must live only inactiveProps/inactivePropsto avoid ambiguous overrides. Becausetext-mutedis present in the base string, both classes end up on the active tab element; whichever Tailwind emit…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 7 findings (5 with multi-model agreement)
web/src/components/layout/AppShell.tsx:113web/src/components/layout/AppShell.tsx:187web/src/components/layout/AppShell.tsx:84web/src/components/layout/AppShell.tsx:35web/src/components/layout/AppShell.tsx:1752 single-model findings (lower confidence)
web/src/components/layout/AppShell.tsx:103web/src/components/layout/AppShell.tsx:180Per-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
Server-side enforcement is intact — this is purely a client-side nav/UI change over React, with text content auto-escaped (no
dangerouslySetInnerHTML), no new API calls, no new user-controlled data flowing into URLs, storage, or templates.VERDICT: No material issues found
AppShell.tsx:38) is UI convenience only; verified the server still gates/settingsbehindrequireAuth()+requireAdmin()ininternal/api/api.go:158andinternal/api/settings.go:22, matching the pre-existing pattern the removed comment described. No authz regression.displayNameis rendered as plain JSX text (AppShell.tsx:128,152), not viadangerouslySetInnerHTMLor a URL/attribute sink — React escapes it, so no XSS vector even though it's user-controlled.AppShell.tsx:141-146) and popover use local component state only; no new storage, no new endpoints, no tokens or secrets touched.useMatchRoute) and section list are static, hard-coded arrays — no injection surface.Nothing here changes trust boundaries, request handling, or credential flow — it's a pure client-side layout/nav refactor.
🎯 Correctness — Minor issues
Both findings confirmed against the actual code.
publicGardenRoute(g/$token) is a child ofrootRoutewhose component isAppShell, has no auth guard, andPublicGardenPage.tsx:56hardcodes the sameh-[calc(100dvh-8rem)]as the editor — butAppShell.tsx:35only exemptsgardens/$gardenIdfromshowBottomNav, so a signed-in user on their own share link gets the overlay. TheonLogoutcatch block does callsetOpen(false)at line 113, closing the popover that shows the retry state described in the adjacent comment.VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:35— Bottom-nav suppression (inEditor) only matchesgardens/$gardenId; it doesn't exemptg/$token, which is also a full-screen canvas route (router.tsx:137-141, nested underAppShellviarootRoute, no auth guard so a signed-in user keeps their session there).PublicGardenPage.tsx:56hardcodesh-[calc(100dvh-8rem)], the same calcGardenEditorPage.tsx:405uses but is correctly exempted for viainEditor. For a signed-in user on their own share link,showBottomNavbecomestrue, addingpb-[calc(4.5rem+env(safe-area-inset-bottom))]tomainand afixedBottomNav(z-20) that the page's fixed-height canvas doesn't leave room for — the bottom of the pan/zoom canvas ends up under the tab bar. Fix: foldg/$tokeninto the full-screen check, or make the page's height calc account for the bottom bar.web/src/components/layout/AppShell.tsx:113—onLogout's catch branch callssetOpen(false), closing the account popover on a failed sign-out. The comment directly above says the intent is to leave the user in place so "the button re-enables for a retry," but closing the popover hides the "Retry sign out" label (driven bylogout.isError, rendered at line 162) until the user reopens the menu — an extra tap contrary to the comment's stated intent. Fix: dropsetOpen(false)in the catch branch.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:113— InAccountMenu'sonLogout, the catch-block comment ("leave the user where they are ... rather than pretending they're signed out") is directly contradicted by thesetOpen(false)on the next line: a failed sign-out closes the account popover, hiding thelogout.isError-driven "Retry sign out" state the comment describes. The user has to reopen the menu to retry. Either the behavior should not close the popover on error, or the comment needs to be rewritten to describe why closing is still fine — as written it's stale/misleading, which CLAUDE.md flags as worse than no comment. Verified by reading the full function (lines 105-115).web/src/components/layout/AppShell.tsx:85(clearance) vs.:180,187(bar height) —<main>'s bottom padding is a hardcodedpb-[calc(4.5rem+env(safe-area-inset-bottom))], while the actualBottomNavheight is built from separate, independently-chosen values (min-h-[3.25rem]per link,py-1.5,border-t, plus its ownpb-[env(safe-area-inset-bottom)]). Nothing ties these two together — confirmed by reading both blocks. IfBottomNav's height changes later, it's easy to forget the matchingpb-update inAppShell, silently reintroducing the "content hidden behind the bar" bug this PR fixes. Worth a shared constant or a cross-referencing comment.web/src/components/layout/AppShell.tsx:175—BottomNav'ssectionsprop is typed by hand asReadonlyArray<{ to: string; label: string; icon: string }>rather than derived from the actual source of truth, thesectionsarray at lines 8-12 (typeof sections[number]or aPick). Confirmed: the real array also carriesadminOnly, so this is already a partial, hand-copied duplicate of the shape — the kind of thing that silently drifts when a field is added.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, N+1 patterns). This is a small, static nav restructuring — no data fetching, loops, or growth-prone state were touched.
visibleSections = sections.filter(...)(AppShell.tsx:38) re-filters a fixed 3-item array on every render — negligible cost, not worth memoizing.useMatchRoute()(AppShell.tsx:31,35) adds one route-match check per render; this is the standard TanStack Router API for this purpose and not a new subscription pattern beyond what routing already does.AccountMenuandBottomNavare declared as top-level functions (not inline closures insideAppShell), so they don't remount/reset state every render — confirmed by reading the file (AppShell.tsx:100,172).AppShell.tsx:141-146) only mounts the extrafixed inset-0button while the popover isopen, not unconditionally — no persistent listener or extra paint cost when closed.Nothing here rises to a reportable performance regression.
🧯 Error handling & edge cases — Minor issues
Both findings check out against the actual code. No toast is fired anywhere on logout failure,
useLogouthas noonError, and the retry affordance lives exclusively inside the{open && ...}block thatsetOpen(false)unmounts. The route-persistence claim for finding 2 is also confirmed —AppShellis the root layout component (router.tsx:27), so it doesn't remount across navigations, and the backdrop (z-30) sits above both the top bar and bottom nav (z-20).VERDICT: Minor issues
Findings (🧯 error handling & edge cases lens):
web/src/components/layout/AppShell.tsx:113— On logout failure,onLogout's catch block callssetOpen(false), closing the account popover. That popover is the only place the failure is surfaced (title="Sign out failed — try again"and the "Retry sign out" label live inside it, at lines 159/162), and the comment directly above the call still says "leave the user where they are (the button re-enables for a retry)" — the code no longer does that; it hides the retry button instead.useLogout(web/src/lib/auth.ts:86-97) has noonErrorhandler, and no toast fires for this path anywhere inAppShell.tsxorauth.ts, unlikeweb/src/lib/objects.ts, where every mutation callstoast.error(...)on failure — the codebase's established pattern for surfacing async failures. So on a failed sign-out, the menu silently closes with zero visible feedback; the user has to guess something went wrong and manually reopen the menu to see the "Retry sign out" state. The diff's removed pre-PRonLogouthad nosetOpencall and the retry button (then inline, not in a popover) was always visible, so this is a regression introduced by moving to the popover. Fix: don't close the popover on failure (drop thesetOpen(false)in the catch, or replace it with atoast.errorso failure is visible even if the menu later closes).web/src/components/layout/AppShell.tsx:103(AccountMenu) —openstate is never reset on route change.AppShellis the root route layout component (web/src/router.tsx:27), so it doesn't remount on navigation — only the routedOutletcontent changes. The full-screen click-away backdrop (fixed inset-0 z-30, line 144) sits above the bottom nav and top nav links (bothz-20), so while the menu is open, a link tap hits the backdrop and closes the menu instead of navigating. Browser back/forward, however, changes the route directly without going through that click, so the backdrop and menu can remain open (and mounted) over a newly-navigated page, swallowing the next tap. Minor/edge-case, but it's a missing-cleanup gap: nothing tiesopento the route. AuseEffectresettingopenon location change (or closing onLinkclick as well as backdrop click) would close the gap.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
Through the security lens, this change is clean:
visibleSectionsfilters Settings byuser?.isAdmin(AppShell.tsx:38), matching the prior admin-only render. Confirmed the real boundary is server-side:router.tsx:130wires/settingsbeforeLoadtorequireAdmin, which checksme.isAdmin(router.tsx:56-61). The PR description and retained comment correctly frame the hidden link as convenience, not the boundary — no regression and no new client-trust issue.displayName(zod-validatedz.string()in auth.ts:11) is only rendered as React text children (button label, popover "Signed in as …", and theinitialderived viacharAt(0).toUpperCase()). NodangerouslySetInnerHTML, no template interpolation into attributes/markup.displayNameis surfaced (and it was already shownsm:inlinebefore); no tokens, emails-in-headers, or other sensitive fields added.AccountMenu.onLogoutkeeps the same fail-leave-in-place semantics (mutate, then navigate to/loginonly on success), anduseLogout'sonSuccess(auth.ts:90-95) nulls themecache and drops non-auth query data. The error branch closing the popover is a UX detail, not a security one.backdrop z-30<menu z-40) is sound for click routing; no clickjacking-style interception of the sign-out action.🎯 Correctness — No material issues found
Verdict: No material issues found
Through the correctness lens I verified the change's logic against the actual checked-out code:
matchRoute({ to: '/gardens/$gardenId', fuzzy: false })is correct against the registered route atweb/src/router.tsx:108-118(path: 'gardens/$gardenId').fuzzy:false+ the required$gardenIdparam means the/gardenslist route (router.tsx:101-106, pathgardens) does not match, so the bottom bar correctly shows on the list and hides in the editor. Search params (?focus=) don't affect route matching, so editor links with focus still match. (VerifieduseMatchRouteis a standard TanStack Router hook; the dependency is@tanstack/react-router1.170.18 perweb/package-lock.json:1538.)visibleSections = sections.filter((s) => !s.adminOnly || user?.isAdmin)(AppShell.tsx:38) reproduces the prior behavior: Gardens/Plants for everyone, Settings only whenuser.isAdmin, consistent with therequireAdminguard atrouter.tsx:56-64and theisAdminfield in the user schema (lib/auth.ts:12). Theas constarray is passed intoBottomNav's widerReadonlyArray<{to,label,icon}>param via a variable, so no excess-property check fires — fine.AccountMenu.onLogoutmirrors the oldonLogout(awaitlogout.mutateAsync(), then navigate to/login); on error it closes the popover and leaves the session intact.useLogout(lib/auth.ts:86-97) nulls themecache inonSuccess, which flipsuserto null and hides the bottom nav / swaps to the login link. Correct.fixed inset-0 z-30, menuabsolute right-0 z-40: the menu sits above the backdrop so the Sign out button is clickable; the backdrop closes on outside tap. Correct.min-h-[3.25rem](52px) +py-1.5(12px) +pb-[env(safe-area-inset-bottom)]≈ 64px + safe-area; main addspb-[calc(4.5rem+env(safe-area-inset-bottom))]= 72px + safe-area → ~8px clearance plus the safe area. Content is not hidden. Correct.No semantic, formula, threshold, or logic defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/components/layout/AppShell.tsx:187—BottomNavputstext-mutedin the link's base className and ininactiveProps(:189). This contradicts the file's own documented convention at lines 14–16 ("state-specific and conflicting utilities (text-muted vs text-fg) live in the state props — never in the base — to avoid ambiguous overrides"). The active state usestext-accent-strong(:188), which the basetext-mutedwill fight under concatenation; the desktop links (:56+:57-58) follow the convention correctly by keeping color out ofnavLinkBase. The bottom bar should match: droptext-mutedfrom the base so only the state props carry color.web/src/components/layout/AppShell.tsx:174-176—BottomNav'ssectionsprop is typed as a hand-writtenReadonlyArray<{ to: string; label: string; icon: string }>, a partial re-declaration of thesectionsarray's shape (:8-12, declaredas constwith anadminOnlyfield this type omits). It can silently drift if a field is added or renamed. Prefer deriving the element type (e.g.(typeof sections)[number]and picking the needed fields, or accepting the full objects) so the prop type stays in sync with the source of truth.Both verified by reading
AppShell.tsxin full and comparing the desktop-link path (:52-62) against the bottom-bar path (:183-195).⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens this change is clean. Specifics I verified by reading
AppShell.tsx:visibleSections = sections.filter(...)(line 38) runs every render, butsectionsis a 3-elementas constarray — trivial, not a hot path, and strictly cheaper than the old code which rendered the links plus a separate conditional Settings block.useMatchRoute(line 31) and theinEditorcheck (line 35) are constant-time hook calls per render; no route-table scan leaks into render work.BottomNavandAccountMenuare tiny presentational components;.mapover ≤3 items, no allocations worth flagging.pb-[calc(...)]padding are static CSS, not runtime work.One non-blocking note (minor, not material): the new
BottomNav(line 180) adds a second always-onbackdrop-blursurface (the header at line 42 is the first). Two fixed/stickybackdrop-blurlayers re-sampling the backdrop during scroll can cause scroll repaint cost on low-end mobile. If scroll jank shows up in profiling later, drop the bottom bar to a solidbg-surface(no blur); the header blur is the more valuable one. Not worth changing preemptively.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/components/layout/AppShell.tsx:113— On logout failure thecatchcallssetOpen(false), closing the popover and hiding the only place the error/retry affordance lives (logout.isError→ "Retry sign out" at line 162). The header account-toggle button (lines 121-135) carries no error state, so after a failed sign-out the user gets zero feedback and the retry button is now buried behind reopening the menu. The original code left the button visible sologout.isErrorcould surface. The comment at line 112 even says "the button re-enables for a retry," but closing the popover contradicts that. Fix: don'tsetOpen(false)on error (keep the popover open so the "Retry sign out" label is visible), or surface the error on the toggle button itself (e.g. atitle/aria-label + visual cue driven bylogout.isError) before closing.Other edge cases check out:
initial(line 117) safely falls back to'·'for empty/whitespacedisplayName;visibleSectionsfilter (line 38) handlesuserbeing undefined correctly;showBottomNavisfalsewhen!user, soBottomNavisn't rendered with an empty list; the popover closes cleanly on outside tap via the z-30 backdrop (lines 141-146) covering the z-20 header.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This PR is purely a UI/layout refactor — moving navigation from a desktop-oriented top bar to a mobile-first bottom tab bar + slim top bar. Through the security lens, there's nothing material to report:
/gardens,/plants,/settings,/gardens/$gardenId,/login) are hardcoded constants. Thesectionsarray and allLinktoprops are static strings; no user input reaches a URL, path, command, template, or query.displayName:user.displayNameis rendered only as JSX text content (lines 128, 152 of AppShell.tsx), which React escapes automatically. It never reaches adangerouslySetInnerHTML,href,src, CSS className, or JavaScript context. The computedinitialis produced viacharAt(0).toUpperCase()— safe.user?.isAdmin, and the PR explicitly (and correctly) notes the server is the security boundary. No authn/authz gap introduced.useNavigate) is to the hardcoded/loginpath only.<button>(z-30) is a standard, safe pattern that avoids a persistent document listener; it doesn't create a clickjacking vector.🎯 Correctness — Blocking issues found
Blocking issues found
web/src/components/layout/AppShell.tsx:187— TheBottomNavLinkbaseclassNameincludestext-muted, butactivePropsonly addstext-accent-strong. The file's own comment (line 14-16) explains that TanStack Router concatenates base + state classes, so conflicting color utilities must live only inactiveProps/inactivePropsto avoid ambiguous overrides. Becausetext-mutedis present in the base string, both classes end up on the active tab element; whichever Tailwind emits later in the CSS wins. If.text-mutedhappens to follow.text-accent-strongin the generated stylesheet (determined by scan order, not source order), the active tab will stay muted instead of highlighted.Fix: Remove
text-mutedfrom the baseclassNameand add it toinactiveProps.className.web/src/components/layout/AppShell.tsx:113— In theonLogoutcatch handler,setOpen(false)closes the account popover when logout fails. The comment two lines above explicitly says the intent is to "leave the user where they are (the button re-enables for a retry)." Closing the menu hides the button entirely, so the user cannot retry without first reopening the menu; the error state (logout.isError→ "Retry sign out") is no longer visible.Fix: Remove
setOpen(false)from the catch block so the menu stays open and the retry button remains accessible.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/layout/AppShell.tsx:84— The<main>element uses a template-stringclassNamefor conditional padding instead of the project'scn()utility. The codebase consistently usescn()for conditional class composition (e.g.,web/src/components/ui/Button.tsx,web/src/editor/ChatPanel.tsx). This should beclassName={cn('mx-auto w-full max-w-5xl flex-1 px-4 py-6', showBottomNav && 'pb-[calc(4.5rem+env(safe-area-inset-bottom))] md:pb-6')}to stay consistent and avoid Tailwind conflict ambiguity.web/src/components/layout/AppShell.tsx:187—BottomNavputstext-mutedin the baseclassNamewhile also repeating it ininactiveProps. The comment at lines 14–19 explicitly documents the project convention that state-specific utilities belong only inactiveProps/inactivePropsto avoid ambiguous overrides when TanStack Router concatenates classes. The base should droptext-muted(and any other color/state utilities) so the inactive state owns it, matching the pattern used for the desktop nav links.⚡ Performance — No material issues found
No material issues found.
AppShell.tsx:35—matchRouteis called on every render, butuseMatchRoutereturns a stable function and the match check itself is cheap; this is idiomatic TanStack Router usage and not a hot path concern.AppShell.tsx:38—visibleSectionsis re-filtered on every render, but the source array has at most 3 elements; negligible cost.🧯 Error handling & edge cases — Minor issues
Minor issues
AppShell.tsx:113— Account menu closes on logout error, hiding retry feedback:In
AccountMenu.onLogout, the catch block callssetOpen(false)whenlogout.mutateAsync()fails. Becauselogout.isErrordrives the button label ("Retry sign out"), closing the menu immediately means the user never sees that the request failed or the retry prompt. They must reopen the menu to discover the error state.Fix: Remove
setOpen(false)from the catch block so the menu stays open and the error state remains visible for an immediate retry. (If logout succeeds butnavigatethrows, the component will unmount via theuseLogout.onSuccesscache clear anyway.)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
adminOnlyfilter (user?.isAdmin) only hides the link as a convenience, while the API still enforcesrequireAdmin(internal/api/api.go:158,internal/service/instance_settings.go:21) and returns 403 for non-admins — exactly the security posture the prior inline link documented, preserved here.displayNameas text content (initial viadisplayName.trim().charAt(0)), with nodangerouslySetInnerHTMLor HTML interpolation anywhere in the diff, so no XSS surface is introduced through the new UI.logout.mutateAsync, leaves the user in place on failure, navigates to/loginon success); the popover merely relocates the control.<button>withz-30over the menu (z-40) ordering — purely a UI concern, not a security one.I verified the
/settingsserver gate atinternal/api/api.go:158and therequireAdminservice check atinternal/service/instance_settings.go:21-34, and confirmed the repo has nodangerouslySetInnerHTMLusage inweb/src.🎯 Correctness — Minor issues
The finding is confirmed against the actual code:
AppShell.tsx:35-36—inEditoronly matches/gardens/$gardenId;/g/$tokenis not excluded, soshowBottomNavis true for any signed-in viewer on the public share route.router.tsx:137-141—publicGardenRouteisg/$token, a separate full-screen canvas route with no auth guard (so logged-in users can reach it).PublicGardenPage.tsx:56— the container usesh-[calc(100dvh-8rem)], a fixed viewport-relative height that only accounts for the top header. The bottom bar (BottomNav,fixed inset-x-0 bottom-0 z-20, ~52px viamin-h-[3.25rem]plus safe-area padding) overlays the bottom of that canvas.Correctness review
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:35— bottom bar overlaps the public-garden canvas for logged-in viewers.inEditoronly matches/gardens/$gardenId, so the bottom bar is correctly hidden in the editor — but the other full-screen canvas route,/g/$token(publicGardenRoute,web/src/router.tsx:137), is not excluded.showBottomNav = !!user && !inEditoris true for any signed-in visitor viewing a share link on mobile.PublicGardenPagesizes its container withh-[calc(100dvh-8rem)](web/src/pages/PublicGardenPage.tsx:56), which only subtracts the top header; the new ~52px+safe-area bottom bar then sits on top of the canvas bottom, obscuring it. Before this PR there was no bottom bar anywhere, so that calc was correct; this change regresses it for the logged-in-on-a-shared-link case. Fix: extend the hide condition to the public route, e.g.const inFullScreen = !!matchRoute({ to: '/gardens/$gardenId', fuzzy: false }) || !!matchRoute({ to: '/g/$token', fuzzy: false })and keyshowBottomNavoff that.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:187— TheBottomNavLink's base className includestext-muted, which both duplicatesinactiveProps(line 189:text-muted hover:text-fg) and conflicts withactiveProps(line 188:text-accent-strong) when active. TanStack concatenates base + state classes, so an active tab ends up with bothtext-mutedandtext-accent-strong— order-dependent and ambiguous. This directly contradicts the in-file convention comment at lines 14–16 ("state-specific and conflicting utilities … live in the state props — never in the base") and the desktop links above, whosenavLinkBase(line 17) deliberately carries no color for exactly this reason. Fix: droptext-mutedfrom the base on line 187 (it's already ininactiveProps), matching the desktop links' pattern.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens. This is a small presentational shell change; nothing here introduces hot-path work, unbounded growth, N+1 queries, or avoidable quadratic behavior.
Things I checked in
web/src/components/layout/AppShell.tsx:sections.filter(...)at line 38 and the.maprenders (lines 52, 183) operate on a 3-element constant array — trivial, not hot.useMatchRoute(line 31) anduseMe(line 29) are called unconditionally on every render as is correct for hooks; their cost is negligible and unchanged in character from the prioruseNavigate/useLogoutcalls.<button>(line 141) only mounts while the account popover isopen; it's not on any persistent path and adds no continuous cost.backdrop-blurbottom bar (line 180) does force a compositing layer and can raise repaint cost during scroll on mobile, but the existing sticky top bar already usesbackdrop-blur(line 42), so this doesn't introduce a new class of cost — and it'smd:hidden, so desktop is unaffected. Not material enough to flag as a regression.No performance findings to report.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:109-114— logout-failure retry affordance is hidden. InAccountMenu.onLogout's catch block,setOpen(false)(line 113) dismisses the popover on error. The "Retry sign out" label and the disabled-state affordance both live inside that popover — the popover only renders whenopenis true (line 137), and the button whose label flips to "Retry sign out" vialogout.isErroris at lines 154-163. So after a transient logout failure the menu is dismissed and the user sees no indication anything went wrong or that a retry is available; they must happen to reopen the menu to discover the retry button. The comment at lines 110-112 ("the button re-enables for a retry") describes the old inline-button behavior that the diff removed — the button no longer stays in place, so the comment is now stale and the behavior it claims is gone. This is a real unhappy-path regression: a transient logout failure now looks like nothing happened. Verified againstuseLogoutinauth.ts:86-97(the mutation'sisError/isPendingstate drives the label, but only renders whileopen). Suggested fix: dropsetOpen(false)from the catch so the menu stays open and the "Retry sign out" button remains visible; or surface the failure via a toast and leave the menu open. Update the stale comment either way.The remaining unhappy paths (pending logout with menu close via the backdrop, mutation resolving after popover unmount on success) are benign — TanStack Query mutations and
useNavigatetolerate component unmount, andshowBottomNav/visibleSectionsare only rendered whenuseris non-null, so the null-user branch can't reachBottomNav. ThedisplayName.trim().charAt(0).toUpperCase() || '·'fallback at line 117 correctly handles empty/whitespace-only names (the||catches the empty string fromcharAt(0)), matchinguserSchemainauth.ts:8-16wheredisplayName: z.string()permits empty strings. Nothing else material in this lens.opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This PR is a pure UI/layout refactor. From the Security lens:
user?.isAdminclient-side filter for the Settings link is explicitly documented as UI convenience; the server'srequireAdmingate remains the actual security boundary. Nothing here weakens that.Linktoprops are hardcoded constants; no user-controlled URLs or path injection.displayName:displayNameis rendered in standard JSX (<span>{displayName}</span>), which React escapes. Theinitialderivation (charAt(0).toUpperCase()) operates on az.string()-validated field and is likewise escaped.The
useMatchRoutecall is used solely for chrome layout (hiding the bottom bar in the editor), not for any permission decision. Logout handling is unchanged from the prior implementation.🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/layout/AppShell.tsx:84-86— The<main>element uses a template-literalclassNamefor conditional padding instead of the project'scn()utility (confirmed via grep thatcn()is used in ~27 other components for exactly this pattern). Template literals force readers to parse string concatenation and risk Tailwind class-scanning misses;cn('mx-auto w-full max-w-5xl flex-1 px-4 py-6', showBottomNav && 'pb-[calc(4.5rem+env(safe-area-inset-bottom))] md:pb-6')is consistent with the rest of the codebase and easier to maintain.⚡ Performance — Minor issues
Minor issues
web/src/components/layout/AppShell.tsx:180— The mobile bottom nav addsbackdrop-bluralongside the header's existing blur, creating two simultaneous expensive compositor layers on mobile. During scroll, the browser must continuously capture and blur content behind both fixed elements, which can cause frame drops and increased battery usage on lower-end devices. The visual benefit is marginal becausebg-surface/95is already nearly opaque.Fix: Remove
backdrop-blurfrom the bottom nav (or switch to an opaquebg-surfacebackground).🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling & edge-cases lens, the PR is clean:
displayNameedge cases are handled.displayName.trim().charAt(0).toUpperCase() || '·'correctly falls back to·for empty or whitespace-only strings.matchRoutefailure modes are safe.!!matchRoute(...)coerces any falsy return tofalse; the route string is statically typed by TanStack Router.try/catcharoundmutateAsync+navigatebehaves identically to the old inline code;setOpen(false)in the catch is benign.user?.isAdminis used safely;visibleSectionsis only rendered whenuseris truthy.useEffector document listener leaks.Nothing in my area.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.