Mobile-first app shell: bottom tab nav + slim top bar (#98) #109

Merged
steve merged 2 commits from feat/mobile-shell into main 2026-07-22 05:03:54 +00:00
Owner

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.ai/code/session_01H3zbym8Doka2d7D48maSgZ

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
steve added 1 commit 2026-07-22 04:51:39 +00:00
Mobile-first app shell: bottom tab nav + slim top bar (#98)
Build image / build-and-push (push) Successful in 10s
Gadfly review (reusable) / review (pull_request) Successful in 10m32s
Adversarial Review (Gadfly) / review (pull_request) Successful in 10m33s
9f434a801a
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

🪰 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

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>
steve added 1 commit 2026-07-22 05:02:08 +00:00
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
gitea-actions bot reviewed 2026-07-22 05:02:11 +00:00
gitea-actions bot left a comment

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

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -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 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>
@@ -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:175BottomNav'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>
@@ -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 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>
@@ -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 — 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…

🪰 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>

🪰 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
2 single-model findings (lower confidence)
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
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

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 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:113onLogout'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:175BottomNav'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.

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:

  • 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.
🎯 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 detectionmatchRoute({ 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 filteringvisibleSections = 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 flowAccountMenu.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:187BottomNav 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-176BottomNav'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

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:

  • 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.
🎯 Correctness — Blocking issues found

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.

🧹 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:187BottomNav 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:35matchRoute 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:38visibleSections 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.)
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

  • 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-36inEditor 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-141publicGardenRoute 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

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:

  • 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.

🎯 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 main 2026-07-22 05:03:54 +00:00
steve deleted branch feat/mobile-shell 2026-07-22 05:03:54 +00:00
Sign in to join this conversation.