Address Gadfly review on #6: logout errors, register gating, guard errors

Fixes from the PR #25 adversarial review (graded 23 real / 5 false positive):

Error handling
- onLogout wraps mutateAsync in try/catch: a failed logout no longer
  becomes an unhandled rejection; the user stays put (session still valid)
  and the button offers "Retry sign out" (5 models flagged this).
- Root errorComponent (RouteError): a non-401 /auth/me failure in a
  beforeLoad guard now shows a recoverable "Try again" screen instead of
  blanking.
- Forms drop noValidate, restoring native required/type=email/minLength
  checks before hitting the server.

Correctness
- RegisterPage gates on providers.isPending/isError before rendering, so
  the form no longer briefly appears (submittable) on an SSO-only server.
- TextField id falls back to name then a useId() value, so the label/hint
  associations hold even if a caller omits both.

Maintainability
- Single safeRedirectPath (lib/redirect.ts) replaces the duplicated
  safeRedirect/safeInternalPath (4 models).
- Generic ApiError helpers (apiErrorCode, errorMessage) moved to lib/api.ts;
  LoginPage builds the OIDC URL from the exported API_BASE.
- Divider moved to components/ui/. errorMessage doc clarified.

Verified again in a real browser: register -> /gardens; Sign out -> /login.
tsc --noEmit and vite build clean.

Not changed (graded, with rationale): OIDC deep-link redirect isn't
preserved (needs backend state threading — follow-up); 60s me staleTime in
guards (server is authoritative); the login/register onSubmit catches are
the intended react-query pattern (errors render via mutation state).

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

web/src/components/RouteError.tsx

@@ -0,0 +1,17 @@
+import { useRouter } from '@tanstack/react-router'
+import { Button } from '@/components/ui/Button'
+import { errorMessage } from '@/lib/api'
+
+// Root error boundary: if a route's beforeLoad/loader throws something other
+// than a redirect (e.g. /auth/me fails with a network error or a 500), show a
+// recoverable message instead of a blank screen. Retrying re-runs the guards.
+export function RouteError({ error }: { error: Error }) {
+  const router = useRouter()
+  return (
+    <div className="mx-auto flex min-h-[60vh] w-full max-w-sm flex-col items-center justify-center gap-4 text-center">
+      <h1 className="text-lg font-semibold text-fg">Something went wrong</h1>
+      <p className="text-sm text-muted">{errorMessage(error, 'An unexpected error occurred.')}</p>
+      <Button onClick={() => router.invalidate()}>Try again</Button>
+    </div>
+  )
+}

web/src/components/layout/AppShell.tsx

@@ -21,8 +21,14 @@ export function AppShell() {
   const user = me.data
 
   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.
+    }
   }
 
   return (
@@ -55,9 +61,10 @@ export function AppShell() {
                 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…' : 'Sign out'}
+                {logout.isPending ? 'Signing out…' : logout.isError ? 'Retry sign out' : 'Sign out'}
               </button>
             </div>
           ) : (

web/src/components/ui/Divider.tsx

@@ -0,0 +1,12 @@
+import type { ReactNode } from 'react'
+
+/** A horizontal rule with centered label text (e.g. "or" between auth options). */
+export function Divider({ children }: { children: ReactNode }) {
+  return (
+    <div className="flex items-center gap-3 text-xs uppercase tracking-wide text-muted">
+      <span className="h-px flex-1 bg-border" />
+      {children}
+      <span className="h-px flex-1 bg-border" />
+    </div>
+  )
+}

web/src/components/ui/TextField.tsx

@@ -1,4 +1,4 @@
-import { forwardRef, type InputHTMLAttributes } from 'react'
+import { forwardRef, useId, type InputHTMLAttributes } from 'react'
 import { cn } from '@/lib/cn'
 
 interface TextFieldProps extends InputHTMLAttributes<HTMLInputElement> {
@@ -7,12 +7,14 @@ interface TextFieldProps extends InputHTMLAttributes<HTMLInputElement> {
 }
 
 // A labelled text input. text-base (16px) is deliberate: smaller fonts make iOS
-// Safari zoom on focus. The label's htmlFor falls back to the field name.
+// Safari zoom on focus. The field id falls back to the name, then to a generated
+// id, so the label/hint associations always hold.
 export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(function TextField(
   { label, hint, id, name, className, ...props },
   ref,
 ) {
-  const inputId = id ?? name
+  const generatedId = useId()
+  const inputId = id ?? name ?? generatedId
   const hintId = hint ? `${inputId}-hint` : undefined
   return (
     <div className="flex flex-col gap-1.5">

web/src/lib/api.ts

@@ -5,7 +5,8 @@
 // editor (per DESIGN.md § Sync) sends each PATCH/DELETE with a row `version` and,
 // on a 409, reads the *current* row back out of `ApiError.body` to reconcile.
 
-const BASE = '/api/v1'
+/** Base path for the versioned JSON API; also used to build server-nav links. */
+export const API_BASE = '/api/v1'
 
 /** Error thrown for any non-2xx response (or a network failure, status 0). */
 export class ApiError extends Error {
@@ -44,7 +45,7 @@ export interface RequestOptions {
 }
 
 function buildUrl(path: string, params?: Params): string {
-  const url = BASE + path
+  const url = API_BASE + path
   if (!params) return url
   const qs = new URLSearchParams()
   for (const [k, v] of Object.entries(params)) {
@@ -112,6 +113,25 @@ export async function apiFetch<T>(path: string, opts: RequestOptions = {}): Prom
   return payload as T
 }
 
+/** The server error code from an ApiError body ({error:{code,message}}), if any. */
+export function apiErrorCode(err: unknown): string | null {
+  if (err instanceof ApiError && err.body && typeof err.body === 'object') {
+    const e = (err.body as { error?: { code?: unknown } }).error
+    if (e && typeof e.code === 'string') return e.code
+  }
+  return null
+}
+
+/**
+ * A user-facing message for a caught request error. An ApiError carries the
+ * server's own message (already user-friendly), so that wins; `fallback` covers
+ * everything else (unexpected throws).
+ */
+export function errorMessage(err: unknown, fallback = 'Something went wrong.'): string {
+  if (err instanceof ApiError) return err.message
+  return fallback
+}
+
 /** Convenience verbs over apiFetch. */
 export const api = {
   get: <T>(path: string, opts?: Omit<RequestOptions, 'method' | 'body'>) =>

web/src/lib/auth.ts

@@ -95,18 +95,3 @@ export function useLogout() {
     },
   })
 }
-
-/** The server error code from an ApiError body ({error:{code,message}}), if any. */
-export function apiErrorCode(err: unknown): string | null {
-  if (err instanceof ApiError && err.body && typeof err.body === 'object') {
-    const e = (err.body as { error?: { code?: unknown } }).error
-    if (e && typeof e.code === 'string') return e.code
-  }
-  return null
-}
-
-/** A user-facing message for a caught request error. */
-export function errorMessage(err: unknown, fallback = 'Something went wrong.'): string {
-  if (err instanceof ApiError) return err.message
-  return fallback
-}

web/src/lib/redirect.ts

@@ -0,0 +1,14 @@
+// Where to land after auth. The router guard stashes the intended destination
+// in ?redirect=, and the login page reads it back; both go through this so a
+// caller-controlled value can never send the user to an external origin.
+
+const DEFAULT_DESTINATION = '/gardens'
+
+/**
+ * Return dest only if it is a same-origin absolute path (starts with a single
+ * '/'); otherwise fall back to the gardens list. Rejects protocol-relative
+ * ('//evil.com') and absolute ('https://…') URLs.
+ */
+export function safeRedirectPath(dest: string | undefined, fallback = DEFAULT_DESTINATION): string {
+  return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : fallback
+}

web/src/pages/LoginPage.tsx

@@ -1,14 +1,17 @@
-import { useState, type FormEvent, type ReactNode } from 'react'
+import { useState, type FormEvent } from 'react'
 import { Link, useNavigate, useSearch } from '@tanstack/react-router'
 import { AuthCard } from '@/components/auth/AuthCard'
 import { Alert } from '@/components/ui/Alert'
 import { Button, buttonClasses } from '@/components/ui/Button'
+import { Divider } from '@/components/ui/Divider'
 import { TextField } from '@/components/ui/TextField'
-import { errorMessage, useLogin, useProviders } from '@/lib/auth'
+import { API_BASE, errorMessage } from '@/lib/api'
+import { useLogin, useProviders } from '@/lib/auth'
+import { safeRedirectPath } from '@/lib/redirect'
 
 // Server-initiated redirect (not a fetch): the browser navigates to the Go
 // handler, which 302s to the IdP.
-const OIDC_LOGIN_URL = '/api/v1/auth/oidc/login'
+const OIDC_LOGIN_URL = `${API_BASE}/auth/oidc/login`
 
 // Messages for the ?error= codes the OIDC callback redirects back with.
 const callbackErrors: Record<string, string> = {
@@ -20,11 +23,6 @@ const callbackErrors: Record<string, string> = {
   oidc_conflict: 'That single sign-on identity conflicts with an existing account.',
 }
 
-// Only follow same-origin absolute paths, never a caller-controlled external URL.
-function safeRedirect(dest: string | undefined): string {
-  return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens'
-}
-
 export function LoginPage() {
   const search = useSearch({ from: '/login' })
   const navigate = useNavigate()
@@ -33,7 +31,7 @@ export function LoginPage() {
   const [email, setEmail] = useState('')
   const [password, setPassword] = useState('')
 
-  const dest = safeRedirect(search.redirect)
+  const dest = safeRedirectPath(search.redirect)
   const callbackError = search.error ? (callbackErrors[search.error] ?? callbackErrors.oidc) : null
 
   async function onSubmit(e: FormEvent) {
@@ -66,7 +64,7 @@ export function LoginPage() {
         {oidc && local && <Divider>or</Divider>}
 
         {local && (
-          <form onSubmit={onSubmit} className="flex flex-col gap-3" noValidate>
+          <form onSubmit={onSubmit} className="flex flex-col gap-3">
             <TextField
               label="Email"
               name="email"
@@ -110,13 +108,3 @@ export function LoginPage() {
     </AuthCard>
   )
 }
-
-function Divider({ children }: { children: ReactNode }) {
-  return (
-    <div className="flex items-center gap-3 text-xs uppercase tracking-wide text-muted">
-      <span className="h-px flex-1 bg-border" />
-      {children}
-      <span className="h-px flex-1 bg-border" />
-    </div>
-  )
-}

web/src/pages/RegisterPage.tsx

@@ -4,7 +4,8 @@ import { AuthCard } from '@/components/auth/AuthCard'
 import { Alert } from '@/components/ui/Alert'
 import { Button } from '@/components/ui/Button'
 import { TextField } from '@/components/ui/TextField'
-import { apiErrorCode, errorMessage, useProviders, useRegister } from '@/lib/auth'
+import { apiErrorCode, errorMessage } from '@/lib/api'
+import { useProviders, useRegister } from '@/lib/auth'
 
 const MIN_PASSWORD = 8
 
@@ -26,8 +27,24 @@ export function RegisterPage() {
     }
   }
 
+  // Don't render the form until we know local registration is offered — otherwise
+  // it briefly appears (and is submittable) on an SSO-only server.
+  if (providers.isPending) {
+    return (
+      <AuthCard title="Create account">
+        <p className="text-sm text-muted">Loading…</p>
+      </AuthCard>
+    )
+  }
+  if (providers.isError) {
+    return (
+      <AuthCard title="Create account">
+        <Alert>Could not load sign-in options. Please refresh.</Alert>
+      </AuthCard>
+    )
+  }
   // Local registration is only meaningful when local auth is enabled.
-  if (providers.isSuccess && !providers.data.local) {
+  if (!providers.data.local) {
     return (
       <AuthCard title="Create account">
         <div className="flex flex-col gap-4">
@@ -52,7 +69,7 @@ export function RegisterPage() {
           </Link>
         </div>
       ) : (
-        <form onSubmit={onSubmit} className="flex flex-col gap-3" noValidate>
+        <form onSubmit={onSubmit} className="flex flex-col gap-3">
           <TextField
             label="Email"
             name="email"

web/src/router.tsx

@@ -6,6 +6,7 @@ import {
 } from '@tanstack/react-router'
 import type { QueryClient } from '@tanstack/react-query'
 import { AppShell } from '@/components/layout/AppShell'
+import { RouteError } from '@/components/RouteError'
 import { LoginPage } from '@/pages/LoginPage'
 import { RegisterPage } from '@/pages/RegisterPage'
 import { GardensPage } from '@/pages/GardensPage'
@@ -13,19 +14,25 @@ import { GardenEditorPage } from '@/pages/GardenEditorPage'
 import { PlantsPage } from '@/pages/PlantsPage'
 import { meQueryOptions } from '@/lib/auth'
 import { queryClient } from '@/lib/queryClient'
+import { safeRedirectPath } from '@/lib/redirect'
 
 interface RouterContext {
   queryClient: QueryClient
 }
 
-const rootRoute = createRootRouteWithContext<RouterContext>()({ component: AppShell })
+const rootRoute = createRootRouteWithContext<RouterContext>()({
+  component: AppShell,
+  // A beforeLoad/loader failure that isn't a redirect (e.g. /auth/me errors with
+  // a 500 or the network drops) lands here instead of a blank screen.
+  errorComponent: RouteError,
+})
 
 // requireAuth: resolve the current user (shared cache with useMe); send anyone
-// unauthenticated to /login, remembering where they were headed.
+// unauthenticated to /login, remembering the path they were headed to.
-async function requireAuth(context: RouterContext, href: string) {
+async function requireAuth(context: RouterContext, path: string) {
   const me = await context.queryClient.ensureQueryData(meQueryOptions)
   if (!me) {
-    throw redirect({ to: '/login', search: { redirect: href } })
+    throw redirect({ to: '/login', search: { redirect: path } })
   }
 }
 
@@ -37,10 +44,6 @@ async function requireGuest(context: RouterContext, redirectTo: string) {
   }
 }
 
-function safeInternalPath(dest: string | undefined): string {
-  return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens'
-}
-
 const indexRoute = createRoute({
   getParentRoute: () => rootRoute,
   path: '/',
@@ -65,7 +68,7 @@ const loginRoute = createRoute({
     if (typeof search.error === 'string') out.error = search.error
     return out
   },
-  beforeLoad: ({ context, search }) => requireGuest(context, safeInternalPath(search.redirect)),
+  beforeLoad: ({ context, search }) => requireGuest(context, safeRedirectPath(search.redirect)),
   component: LoginPage,
 })