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
Showing only changes of commit 79f03acea8 - Show all commits
+31 -19
View File
@@ -1,7 +1,8 @@
import { useState } from 'react'
import { Link, Outlet, useMatchRoute, 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'
// 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.
@@ -11,6 +12,8 @@ const sections = [
{ 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.
@@ -30,10 +33,13 @@ export function AppShell() {
const user = me.data
const matchRoute = useMatchRoute()
// The garden editor route ('/gardens/$gardenId') runs full-screen. Fuzzy:false
// so the '/gardens' list itself still shows the bottom bar.
// 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 showBottomNav = !!user && !inEditor
const inPublicGarden = !!matchRoute({ to: '/g/$token', fuzzy: false })
const showBottomNav = !!user && !inEditor && !inPublicGarden
const visibleSections = sections.filter((s) => !s.adminOnly || user?.isAdmin)
@@ -79,11 +85,13 @@ export function AppShell() {
</nav>
</header>
{/* Bottom bar clearance on mobile so content isn't hidden behind it. */}
<main
className={`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' : ''
}`}
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>
@@ -101,16 +109,21 @@ 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:
// leave the user where they are (the button re-enables for a retry) rather
// than pretending they're signed out. logout.isError drives the label.
setOpen(false)
// 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.
}
}
2
@@ -169,11 +182,7 @@ function AccountMenu({ displayName }: { displayName: string }) {
}
/** Mobile bottom tab bar for the top-level sections (thumb zone, safe-area aware). */
function BottomNav({
sections,
}: {
sections: ReadonlyArray<{ to: string; label: string; icon: string }>
}) {
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"
@@ -182,9 +191,12 @@ function BottomNav({
<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 min-h-[3.25rem] flex-col items-center justify-center gap-0.5 py-1.5 text-xs font-medium text-muted transition-colors"
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' }}
>