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
+160 -49
View File
@@ -1,12 +1,19 @@
import { Link, Outlet, useNavigate } from '@tanstack/react-router'
import { useEffect, useState } from 'react'
import { Link, Outlet, useMatchRoute, useNavigate, useRouterState } from '@tanstack/react-router'
import { Toaster } from '@/components/ui/toast'
import { useLogout, useMe } from '@/lib/auth'
import { cn } from '@/lib/cn'
const navLinks = [
{ to: '/gardens', label: 'Gardens' },
{ to: '/plants', label: 'Plants' },
// Top-level sections. `icon` is only used by the mobile bottom bar; the desktop
// top bar shows labels alone. Settings is filtered to admins at render.
const sections = [
{ to: '/gardens', label: 'Gardens', icon: '🏡', adminOnly: false },
{ to: '/plants', label: 'Plants', icon: '🌿', adminOnly: false },
{ to: '/settings', label: 'Settings', icon: '⚙️', adminOnly: true },
] as const
type Section = (typeof sections)[number]
// TanStack Router concatenates the base className with activeProps/inactiveProps,
// so state-specific and conflicting utilities (text-muted vs text-fg) live in the
// state props — never in the base — to avoid ambiguous overrides.
@@ -14,73 +21,59 @@ const navLinkBase = 'rounded-md px-3 py-1.5 text-sm font-medium transition-color
const navLinkActive = 'bg-border/60 text-fg'
const navLinkInactive = 'text-muted hover:bg-border/60 hover:text-fg'
/** Top-level chrome: a sticky nav bar plus the routed page in an <Outlet>. */
/**
* Top-level chrome. Mobile-first: a slim top bar (brand + account) with the
* section nav moved to a thumb-reachable bottom tab bar; the desktop breakpoint
* (`md:`) restores the inline top nav. The editor is a full-screen context, so it
* owns the bottom of the screen — the app bottom bar hides there (the brand link
* is the way back to the gardens list), leaving exactly one bottom bar per route.
*/
export function AppShell() {
const me = useMe()
const logout = useLogout()
const navigate = useNavigate()
const user = me.data
const matchRoute = useMatchRoute()
async function onLogout() {
try {
await logout.mutateAsync()
await navigate({ to: '/login' })
} catch {
// The logout request failed, so the session is still valid server-side:
// leave the user where they are (the button re-enables for a retry) rather
// than pretending they're signed out. logout.isError drives the title below.
}
}
// Full-screen canvas contexts own the bottom of the screen: the garden editor
// and the public shared-garden view both render a 100dvh-8rem canvas, so a
// signed-in viewer must not get the app bottom bar overlapping it. Fuzzy:false
// so the '/gardens' list itself still shows the bar.
const inEditor = !!matchRoute({ to: '/gardens/$gardenId', fuzzy: false })
const inPublicGarden = !!matchRoute({ to: '/g/$token', fuzzy: false })
const showBottomNav = !!user && !inEditor && !inPublicGarden
const visibleSections = sections.filter((s) => !s.adminOnly || user?.isAdmin)
return (
<div className="flex min-h-full flex-col">
<header className="sticky top-0 z-10 border-b border-border bg-surface/90 backdrop-blur">
<header className="sticky top-0 z-20 border-b border-border bg-surface/90 backdrop-blur">
<nav className="mx-auto flex max-w-5xl items-center gap-4 px-4 py-3">
<Link to="/gardens" className="text-lg font-semibold text-accent-strong">
🌱 pansy
</Link>
<div className="flex flex-1 items-center gap-1">
{/* Desktop inline section links. Hidden on mobile, where the bottom bar
carries them. */}
<div className="hidden flex-1 items-center gap-1 md:flex">
{user &&
navLinks.map((l) => (
visibleSections.map((s) => (
<Link
key={l.to}
to={l.to}
key={s.to}
to={s.to}
className={navLinkBase}
activeProps={{ className: navLinkActive }}
inactiveProps={{ className: navLinkInactive }}
>
{l.label}
{s.label}
</Link>
))}
{/* Settings is admin-only, matching the server's requireAdmin gate.
A non-admin who typed /settings still gets a 403 from the API — the
hidden link is convenience, not the security boundary. */}
{user?.isAdmin && (
<Link
to="/settings"
className={navLinkBase}
activeProps={{ className: navLinkActive }}
inactiveProps={{ className: navLinkInactive }}
>
Settings
</Link>
)}
</div>
{/* Spacer so account/sign-out sits right on mobile (the desktop links
own flex-1 above). */}
<div className="flex-1 md:hidden" />
{user ? (
<div className="flex items-center gap-2">
<span className="hidden text-sm text-muted sm:inline">{user.displayName}</span>
<button
type="button"
onClick={onLogout}
disabled={logout.isPending}
title={logout.isError ? 'Sign out failed — try again' : undefined}
className="rounded-md px-3 py-1.5 text-sm font-medium text-muted transition-colors hover:text-fg disabled:opacity-60"
>
{logout.isPending ? 'Signing out…' : logout.isError ? 'Retry sign out' : 'Sign out'}
</button>
</div>
<AccountMenu displayName={user.displayName} />
) : (
<Link
to="/login"
@@ -92,11 +85,129 @@ export function AppShell() {
</nav>
</header>
<main className="mx-auto w-full max-w-5xl flex-1 px-4 py-6">
<main
className={cn(
'mx-auto w-full max-w-5xl flex-1 px-4 py-6',
// Clear the fixed bottom bar on mobile so content isn't hidden behind
// it. The 3.5rem must match BottomNav's h-14 (kept adjacent below).
showBottomNav && 'pb-[calc(3.5rem+env(safe-area-inset-bottom))] md:pb-6',
)}
>
<Outlet />
</main>
{showBottomNav && <BottomNav sections={visibleSections} />}
<Toaster />
</div>
)
}
/** Account control: a compact button that toggles a small sign-out popover. On
* desktop the display name shows inline; on mobile it lives inside the popover. */
function AccountMenu({ displayName }: { displayName: string }) {
const logout = useLogout()
const navigate = useNavigate()
const [open, setOpen] = useState(false)
const pathname = useRouterState({ select: (s) => s.location.pathname })
Review

🟠 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>
// Close on any route change, so navigating (bottom nav, browser back) can't
// leave the popover — and its full-screen backdrop — stuck open over the page.
useEffect(() => setOpen(false), [pathname])
async function onLogout() {
try {
await logout.mutateAsync()
await navigate({ to: '/login' })
} catch {
// The logout request failed, so the session is still valid server-side.
// Keep the popover OPEN so the button (now "Retry sign out", driven by
// logout.isError) stays on screen — closing it would hide the only retry
// affordance and pretend nothing went wrong.
}
}
const initial = displayName.trim().charAt(0).toUpperCase() || '·'
return (
<div className="relative">
<button
type="button"
onClick={() => setOpen((v) => !v)}
aria-haspopup="menu"
aria-expanded={open}
className="flex items-center gap-2 rounded-md px-2 py-1.5 text-sm text-muted transition-colors hover:text-fg"
>
<span className="hidden sm:inline">{displayName}</span>
<span
aria-hidden
className="flex size-8 items-center justify-center rounded-full bg-border/60 text-sm font-semibold text-fg"
>
{initial}
</span>
</button>
{open && (
<>
{/* Full-screen click-away backdrop; reliably closes on an outside tap
without a document listener. */}
<button
type="button"
aria-label="Close menu"
className="fixed inset-0 z-30 cursor-default"
onClick={() => setOpen(false)}
/>
<div
role="menu"
className="absolute right-0 z-40 mt-2 w-48 rounded-lg border border-border bg-surface p-1 shadow-lg"
>
<p className="truncate px-3 py-2 text-xs text-muted">
Signed in as <span className="text-fg">{displayName}</span>
</p>
<button
type="button"
role="menuitem"
onClick={onLogout}
disabled={logout.isPending}
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'}
Review

🟡 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>
</button>
</div>
</>
)}
</div>
Review

🟡 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>
)
}
/** Mobile bottom tab bar for the top-level sections (thumb zone, safe-area aware). */
function BottomNav({ sections }: { sections: ReadonlyArray<Section> }) {
return (
<nav
Review

🔴 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>
aria-label="Sections"
className="fixed inset-x-0 bottom-0 z-20 border-t border-border bg-surface/95 pb-[env(safe-area-inset-bottom)] backdrop-blur md:hidden"
>
<ul className="mx-auto flex max-w-5xl items-stretch justify-around">
{sections.map((s) => (
<li key={s.to} className="flex-1">
{/* h-14 matches the clearance reserved on <main> above. Color lives
only in the state props (per the top-bar convention), never the
base, so active/inactive don't fight. */}
<Link
to={s.to}
className="flex h-14 flex-col items-center justify-center gap-0.5 text-xs font-medium transition-colors"
activeProps={{ className: 'text-accent-strong' }}
inactiveProps={{ className: 'text-muted hover:text-fg' }}
>
<span aria-hidden className="text-lg leading-none">
{s.icon}
</span>
{s.label}
</Link>
</li>
))}
</ul>
</nav>
)
}