fix: address final whole-branch review findings for Phase 2B admin panel

Consolidated fix wave for 1 Critical + 6 Important findings from the final
review before merge:

- Critical: marking a booking completed no longer frees its booked_slots
  mirror row or the unique-slot index, closing a double-booking hole
  (new migration 20260812110508, with regression pgTAP tests added first
  and confirmed failing against the old trigger/index).
- uploadSalonImage() now returns the storage-relative path instead of an
  absolute public URL, so products/services.image stays one consistent
  format.
- The product/service Id field is disabled while editing, preventing
  accidental duplicate rows from an upsert keyed on a changed id.
- Cancel / Mark completed in AdminBookings now require window.confirm,
  matching the existing delete-confirmation pattern.
- /admin and any unmatched /admin/* path now redirect to /admin/products
  instead of rendering blank.
- Public signup is disabled in supabase/config.toml, and ProtectedRoute
  adds a defense-in-depth check that the session belongs to the admin
  user id, not just that a session exists.
- Product/service delete now surfaces a friendly message for FK-violation
  errors (23503) via a new friendlyError() helper, instead of a raw
  Postgres error string.

Claude-Session: https://claude.ai/code/session_01NhxA4npAXAxmCDEJt6kxSH

src/components/ProtectedRoute.test.tsx

@@ -34,9 +34,19 @@ describe('ProtectedRoute', () => {
     expect(screen.getByText('Login page')).toBeInTheDocument()
   })
 
-  it('renders the protected content when there is a session', () => {
-    adminAuthMock.mockReturnValue({ session: { user: { id: 'admin' } }, loading: false })
+  it('renders the protected content when there is an admin session', () => {
+    adminAuthMock.mockReturnValue({
+      session: { user: { id: '21c90224-e0b2-412e-a431-8e7f097c1cef' } },
+      loading: false,
+    })
     renderWithRoute()
     expect(screen.getByText('Products page')).toBeInTheDocument()
   })
+
+  it('redirects to /admin/login when the session belongs to a non-admin user', () => {
+    adminAuthMock.mockReturnValue({ session: { user: { id: 'some-other-user-id' } }, loading: false })
+    renderWithRoute()
+    expect(screen.getByText('Login page')).toBeInTheDocument()
+    expect(screen.queryByText('Products page')).not.toBeInTheDocument()
+  })
 })

src/components/ProtectedRoute.tsx

@@ -1,11 +1,20 @@
 import { Navigate, Outlet } from 'react-router-dom'
 import useAdminAuth from '../hooks/useAdminAuth'
 
+// The single admin account's UUID, matched throughout this project's RLS
+// policies. Public signup is disabled (see supabase/config.toml), and RLS
+// blocks every write from a non-admin session regardless, but this check is
+// defense-in-depth: it ensures the admin UI itself never renders for an
+// authenticated session that isn't the admin's, even if signup were ever
+// re-enabled or a session existed for some other reason.
+const ADMIN_USER_ID = '21c90224-e0b2-412e-a431-8e7f097c1cef'
+
 function ProtectedRoute() {
   const { session, loading } = useAdminAuth()
 
   if (loading) return null
   if (!session) return <Navigate to="/admin/login" replace />
+  if (session.user.id !== ADMIN_USER_ID) return <Navigate to="/admin/login" replace />
   return <Outlet />
 }
 

src/lib/friendlyError.test.ts

@@ -0,0 +1,19 @@
+import { describe, it, expect } from 'vitest'
+import { friendlyError } from './friendlyError'
+
+describe('friendlyError', () => {
+  it('maps a foreign-key violation (23503) to a friendly message', () => {
+    expect(
+      friendlyError({
+        code: '23503',
+        message:
+          'update or delete on table "products" violates foreign key constraint "inquiries_product_id_fkey" on table "inquiries"',
+      })
+    ).toBe("This can't be deleted because it's referenced by an existing booking or inquiry.")
+  })
+
+  it('falls back to the raw message for any other error', () => {
+    expect(friendlyError({ code: '23505', message: 'duplicate key value' })).toBe('duplicate key value')
+    expect(friendlyError({ message: 'network error' })).toBe('network error')
+  })
+})

src/lib/friendlyError.ts

@@ -0,0 +1,14 @@
+// Postgres error code for a foreign-key violation. bookings.service_id and
+// inquiries.product_id reference products/services with no cascade, so
+// deleting a referenced row fails with a raw, unfriendly Postgres message
+// (e.g. `update or delete on table "products" violates foreign key
+// constraint "inquiries_product_id_fkey"...`). This maps that case (and any
+// other error) to admin-friendly text.
+const FOREIGN_KEY_VIOLATION = '23503'
+
+export function friendlyError(error: { code?: string; message: string }): string {
+  if (error.code === FOREIGN_KEY_VIOLATION) {
+    return "This can't be deleted because it's referenced by an existing booking or inquiry."
+  }
+  return error.message
+}

src/lib/storage.test.ts

@@ -20,16 +20,16 @@ describe('uploadSalonImage', () => {
     fromMock.mockClear()
   })
 
-  it('uploads to the salon-images bucket and returns the public URL', async () => {
+  it('uploads to the salon-images bucket and returns the storage-relative path', async () => {
     uploadMock.mockResolvedValue({ error: null })
-    getPublicUrlMock.mockReturnValue({ data: { publicUrl: 'https://example.supabase.co/storage/v1/object/public/salon-images/products/edge-control.jpg' } })
 
     const file = new File(['fake'], 'edge-control.jpg', { type: 'image/jpeg' })
-    const url = await uploadSalonImage(file, 'products/edge-control.jpg')
+    const path = await uploadSalonImage(file, 'products/edge-control.jpg')
 
     expect(fromMock).toHaveBeenCalledWith('salon-images')
     expect(uploadMock).toHaveBeenCalledWith('products/edge-control.jpg', file, { upsert: true })
-    expect(url).toBe('https://example.supabase.co/storage/v1/object/public/salon-images/products/edge-control.jpg')
+    expect(getPublicUrlMock).not.toHaveBeenCalled()
+    expect(path).toBe('products/edge-control.jpg')
   })
 
   it('throws when the upload fails', async () => {

src/lib/storage.ts

@@ -1,8 +1,12 @@
 import { supabase } from './supabase'
 
+// Returns the storage-relative path (not a public URL) so products.image /
+// services.image always hold the same format regardless of whether the row
+// was seeded (scripts/seed.ts stores relative paths) or uploaded through the
+// admin panel. Callers that need to render the image should derive the
+// public URL from this path at render time.
 export async function uploadSalonImage(file: File, path: string): Promise<string> {
   const { error } = await supabase.storage.from('salon-images').upload(path, file, { upsert: true })
   if (error) throw error
-  const { data } = supabase.storage.from('salon-images').getPublicUrl(path)
-  return data.publicUrl
+  return path
 }

src/pages/admin/AdminApp.test.tsx

@@ -3,7 +3,7 @@ import { MemoryRouter, Routes, Route } from 'react-router-dom'
 import { describe, it, expect, vi } from 'vitest'
 
 vi.mock('../../hooks/useAdminAuth', () => ({
-  default: () => ({ session: { user: { id: 'admin' } }, loading: false }),
+  default: () => ({ session: { user: { id: '21c90224-e0b2-412e-a431-8e7f097c1cef' } }, loading: false }),
 }))
 vi.mock('../../lib/supabase', () => ({
   supabase: {
@@ -39,4 +39,26 @@ describe('AdminApp routing', () => {
     expect(await screen.findByRole('heading', { name: /^products$/i })).toBeInTheDocument()
     expect(screen.getByRole('link', { name: /bookings/i })).toBeInTheDocument()
   })
+
+  it('redirects /admin to /admin/products when authenticated', async () => {
+    render(
+      <MemoryRouter initialEntries={['/admin']}>
+        <Routes>
+          <Route path="/admin/*" element={<AdminApp />} />
+        </Routes>
+      </MemoryRouter>,
+    )
+    expect(await screen.findByRole('heading', { name: /^products$/i })).toBeInTheDocument()
+  })
+
+  it('redirects an unmatched /admin/* path to /admin/products when authenticated', async () => {
+    render(
+      <MemoryRouter initialEntries={['/admin/no-such-page']}>
+        <Routes>
+          <Route path="/admin/*" element={<AdminApp />} />
+        </Routes>
+      </MemoryRouter>,
+    )
+    expect(await screen.findByRole('heading', { name: /^products$/i })).toBeInTheDocument()
+  })
 })

src/pages/admin/AdminApp.tsx

@@ -1,4 +1,4 @@
-import { Routes, Route } from 'react-router-dom'
+import { Routes, Route, Navigate } from 'react-router-dom'
 import ProtectedRoute from '../../components/ProtectedRoute'
 import AdminLogin from './AdminLogin'
 import AdminLayout from './AdminLayout'
@@ -14,11 +14,13 @@ function AdminApp() {
       <Route path="login" element={<AdminLogin />} />
       <Route element={<ProtectedRoute />}>
         <Route element={<AdminLayout />}>
+          <Route index element={<Navigate to="/admin/products" replace />} />
           <Route path="products" element={<AdminProducts />} />
           <Route path="services" element={<AdminServices />} />
           <Route path="schedule" element={<AdminSchedule />} />
           <Route path="bookings" element={<AdminBookings />} />
           <Route path="inquiries" element={<AdminInquiries />} />
+          <Route path="*" element={<Navigate to="/admin/products" replace />} />
         </Route>
       </Route>
     </Routes>

src/pages/admin/AdminBookings.test.tsx

@@ -33,6 +33,7 @@ describe('AdminBookings', () => {
     eqUpdateMock.mockReset().mockResolvedValue({ error: null })
     updateMock.mockReset().mockReturnValue({ eq: eqUpdateMock })
     fromMock.mockReset().mockReturnValue({ select: selectMock, update: updateMock })
+    vi.spyOn(window, 'confirm').mockReturnValue(true)
   })
 
   it('lists bookings with the related service name', async () => {
@@ -42,22 +43,46 @@ describe('AdminBookings', () => {
     expect(fromMock).toHaveBeenCalledWith('bookings')
   })
 
-  it('cancels a booking', async () => {
+  it('cancels a booking after confirmation', async () => {
     render(<AdminBookings />)
     await screen.findByText('Jane Doe')
 
     await userEvent.click(screen.getByRole('button', { name: /cancel/i }))
 
+    expect(window.confirm).toHaveBeenCalled()
     await waitFor(() => expect(updateMock).toHaveBeenCalledWith({ status: 'cancelled' }))
     expect(eqUpdateMock).toHaveBeenCalledWith('id', 'booking-1')
   })
 
-  it('marks a booking completed', async () => {
+  it('does not cancel a booking when the confirmation is dismissed', async () => {
+    vi.spyOn(window, 'confirm').mockReturnValue(false)
+    render(<AdminBookings />)
+    await screen.findByText('Jane Doe')
+
+    await userEvent.click(screen.getByRole('button', { name: /cancel/i }))
+
+    expect(window.confirm).toHaveBeenCalled()
+    expect(updateMock).not.toHaveBeenCalled()
+  })
+
+  it('marks a booking completed after confirmation', async () => {
     render(<AdminBookings />)
     await screen.findByText('Jane Doe')
 
     await userEvent.click(screen.getByRole('button', { name: /mark completed/i }))
 
+    expect(window.confirm).toHaveBeenCalled()
     await waitFor(() => expect(updateMock).toHaveBeenCalledWith({ status: 'completed' }))
   })
+
+  it('does not mark a booking completed when the confirmation is dismissed', async () => {
+    vi.spyOn(window, 'confirm').mockReturnValue(false)
+    render(<AdminBookings />)
+    await screen.findByText('Jane Doe')
+
+    await userEvent.click(screen.getByRole('button', { name: /mark completed/i }))
+
+    expect(window.confirm).toHaveBeenCalled()
+    expect(updateMock).not.toHaveBeenCalled()
+  })
 })

src/pages/admin/AdminBookings.tsx

@@ -85,8 +85,26 @@ function AdminBookings() {
               <td>
                 {booking.status === 'confirmed' && (
                   <>
-                    <button type="button" onClick={() => setStatus(booking.id, 'cancelled')}>Cancel</button>
-                    <button type="button" onClick={() => setStatus(booking.id, 'completed')}>Mark completed</button>
+                    <button
+                      type="button"
+                      onClick={() => {
+                        if (window.confirm('Cancel this booking? This will free up its slot for other bookings.')) {
+                          void setStatus(booking.id, 'cancelled')
+                        }
+                      }}
+                    >
+                      Cancel
+                    </button>
+                    <button
+                      type="button"
+                      onClick={() => {
+                        if (window.confirm('Mark this booking completed?')) {
+                          void setStatus(booking.id, 'completed')
+                        }
+                      }}
+                    >
+                      Mark completed
+                    </button>
                   </>
                 )}
               </td>

src/pages/admin/AdminProducts.tsx

@@ -1,6 +1,7 @@
 import { useEffect, useState, type FormEvent } from 'react'
 import { supabase } from '../../lib/supabase'
 import { uploadSalonImage } from '../../lib/storage'
+import { friendlyError } from '../../lib/friendlyError'
 import styles from './AdminProducts.module.css'
 
 interface AdminProduct {
@@ -78,12 +79,14 @@ function AdminProducts() {
     if (!window.confirm('Delete this product?')) return
     const { error: deleteError } = await supabase.from('products').delete().eq('id', id)
     if (deleteError) {
-      setError(deleteError.message)
+      setError(friendlyError(deleteError))
       return
     }
     await loadProducts()
   }
 
+  const isEditing = Boolean(form.id) && products.some((p) => p.id === form.id)
+
   return (
     <div className={styles.page}>
       <h1>Products</h1>
@@ -114,7 +117,7 @@ function AdminProducts() {
       </table>
 
       <form className={styles.form} onSubmit={handleSubmit}>
-        <h2>{form.id && products.some((p) => p.id === form.id) ? 'Edit product' : 'New product'}</h2>
+        <h2>{isEditing ? 'Edit product' : 'New product'}</h2>
         <label htmlFor="product-id">
           Id
           <input
@@ -122,6 +125,7 @@ function AdminProducts() {
             value={form.id}
             onChange={(e) => setForm((f) => ({ ...f, id: e.target.value }))}
             required
+            disabled={isEditing}
           />
         </label>
         <label htmlFor="product-name">

src/pages/admin/AdminServices.tsx

@@ -1,6 +1,7 @@
 import { useEffect, useState, type FormEvent } from 'react'
 import { supabase } from '../../lib/supabase'
 import { uploadSalonImage } from '../../lib/storage'
+import { friendlyError } from '../../lib/friendlyError'
 import styles from './AdminServices.module.css'
 
 interface AdminService {
@@ -75,12 +76,14 @@ function AdminServices() {
     if (!window.confirm('Delete this service?')) return
     const { error: deleteError } = await supabase.from('services').delete().eq('id', id)
     if (deleteError) {
-      setError(deleteError.message)
+      setError(friendlyError(deleteError))
       return
     }
     await loadServices()
   }
 
+  const isEditing = Boolean(form.id) && servicesList.some((s) => s.id === form.id)
+
   return (
     <div className={styles.page}>
       <h1>Services</h1>
@@ -109,7 +112,7 @@ function AdminServices() {
       </table>
 
       <form className={styles.form} onSubmit={handleSubmit}>
-        <h2>{form.id && servicesList.some((s) => s.id === form.id) ? 'Edit service' : 'New service'}</h2>
+        <h2>{isEditing ? 'Edit service' : 'New service'}</h2>
         <label htmlFor="service-id">
           Id
           <input
@@ -117,6 +120,7 @@ function AdminServices() {
             value={form.id}
             onChange={(e) => setForm((f) => ({ ...f, id: e.target.value }))}
             required
+            disabled={isEditing}
           />
         </label>
         <label htmlFor="service-name">

supabase/config.toml

@@ -173,7 +173,7 @@ enable_refresh_token_rotation = true
 # Requires enable_refresh_token_rotation = true.
 refresh_token_reuse_interval = 10
 # Allow/disallow new user signups to your project.
-enable_signup = true
+enable_signup = false
 # Allow/disallow anonymous sign-ins to your project.
 enable_anonymous_sign_ins = false
 # Allow/disallow testing manual linking of accounts
@@ -218,7 +218,7 @@ web3 = 30
 
 [auth.email]
 # Allow/disallow new user signups via email to your project.
-enable_signup = true
+enable_signup = false
 # If enabled, a user will be required to confirm any email change on both the old, and new email
 # addresses. If disabled, only the new email is required to confirm.
 double_confirm_changes = true

supabase/migrations/20260812110508_completed_bookings_hold_slot.sql

@@ -0,0 +1,75 @@
+-- Follow-up to 20260812105003 (deferred from the final whole-branch review
+-- before merge). Postgres migrations are append-only history, so this
+-- replaces the trigger function and the partial unique index rather than
+-- editing the already-applied 20260812000002/20260812000003 files.
+--
+-- Problem: 20260812105003 widened bookings.status to allow 'completed', and
+-- the admin panel gained a "Mark completed" action, but sync_booked_slot()
+-- (20260812000003) and bookings_active_slot_idx (20260812000002) still only
+-- treat status = 'confirmed' as "slot occupied". As a result:
+--   1. confirmed -> completed deletes the booked_slots mirror row for that
+--      slot, exactly as if the booking had been cancelled.
+--   2. The partial unique index (where status = 'confirmed') no longer
+--      blocks a second confirmed booking for a now-completed slot.
+-- A completed appointment did happen and must continue to occupy its slot;
+-- only 'cancelled' should free it.
+--
+-- Fix: every place sync_booked_slot() tested status = 'confirmed' now tests
+-- status in ('confirmed', 'completed'), and the partial unique index is
+-- rebuilt with the same predicate.
+create or replace function public.sync_booked_slot()
+returns trigger
+language plpgsql
+security definer
+set search_path = ''
+as $$
+begin
+  if tg_op = 'DELETE' then
+    if old.status in ('confirmed', 'completed') then
+      delete from public.booked_slots
+      where date = old.date and time = old.time
+        and not exists (
+          select 1 from public.bookings b
+          where b.date = old.date and b.time = old.time and b.status in ('confirmed', 'completed')
+        );
+    end if;
+    return old;
+  end if;
+
+  if tg_op = 'UPDATE' then
+    -- Covers both a status change away from 'confirmed'/'completed'
+    -- (cancellation) and a reschedule of a still-occupying booking to a
+    -- different (date, time): either way, the OLD slot may no longer have
+    -- any confirmed/completed booking holding it and its mirror row needs
+    -- to go.
+    if old.status in ('confirmed', 'completed')
+       and (old.date, old.time, old.status) is distinct from (new.date, new.time, new.status) then
+      delete from public.booked_slots
+      where date = old.date and time = old.time
+        and not exists (
+          select 1 from public.bookings b
+          where b.date = old.date and b.time = old.time and b.status in ('confirmed', 'completed')
+        );
+    end if;
+    if new.status in ('confirmed', 'completed') then
+      insert into public.booked_slots (date, time)
+      values (new.date, new.time)
+      on conflict do nothing;
+    end if;
+    return new;
+  end if;
+
+  -- INSERT
+  if new.status in ('confirmed', 'completed') then
+    insert into public.booked_slots (date, time)
+    values (new.date, new.time)
+    on conflict do nothing;
+  end if;
+  return new;
+end;
+$$;
+
+drop index if exists public.bookings_active_slot_idx;
+create unique index bookings_active_slot_idx
+  on public.bookings (date, time)
+  where status in ('confirmed', 'completed');

supabase/tests/database/rls.test.sql

@@ -1,5 +1,5 @@
 begin;
-select plan(42);
+select plan(47);
 
 -- Seed reference data as service_role (bypasses RLS)
 set local role service_role;
@@ -242,8 +242,15 @@ select lives_ok(
   'admin can upload to salon-images'
 );
 
+-- Scoped to a single booking (not a blanket service_id filter): several
+-- other knotless bookings above share a slot across statuses (e.g. two
+-- cancelled bookings for 2026-10-01 2:00 PM), and a blanket update would
+-- try to move both of them to 'completed' at once, colliding on the
+-- occupied-slot unique index below -- that's a test-setup hazard, not the
+-- behavior under test here.
 select lives_ok(
-  $$ update public.bookings set status = 'completed' where service_id = 'knotless' $$,
+  $$ update public.bookings set status = 'completed'
+     where date = '2026-10-04' and time = '1:00 PM' and status = 'confirmed' $$,
   'admin can mark a booking completed'
 );
 
@@ -254,6 +261,39 @@ select throws_ok(
   'booking status is still constrained to known values'
 );
 
+-- A completed appointment actually happened and must keep occupying its
+-- slot: marking a confirmed booking completed must NOT free booked_slots
+-- (regression test for the confirmed -> completed double-booking bug).
+select lives_ok(
+  $$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes, status)
+     values ('knotless', '2026-10-05', '10:00 AM', 'Complete Client', '555-0301', 'complete@example.com', '', 'confirmed') $$,
+  'confirmed booking for the completed-slot test succeeds'
+);
+
+select isnt_empty(
+  $$ select * from public.booked_slots where date = '2026-10-05' and time = '10:00 AM' $$,
+  'booked_slots mirror has an entry for the confirmed slot before completion'
+);
+
+select lives_ok(
+  $$ update public.bookings set status = 'completed'
+     where date = '2026-10-05' and time = '10:00 AM' and status = 'confirmed' $$,
+  'marking the booking completed succeeds'
+);
+
+select isnt_empty(
+  $$ select * from public.booked_slots where date = '2026-10-05' and time = '10:00 AM' $$,
+  'booked_slots mirror row STILL holds the slot after confirmed -> completed'
+);
+
+select throws_ok(
+  $$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes, status)
+     values ('knotless', '2026-10-05', '10:00 AM', 'Double Book Client', '555-0302', 'double@example.com', '', 'confirmed') $$,
+  '23505',
+  null,
+  'a second confirmed booking for a now-completed slot still fails with a uniqueness violation'
+);
+
 select isnt_empty(
   $$ select * from public.notification_failures $$,
   'admin can read notification_failures'