From 77e58c0b92fbc360349bf26c70ceaca0b3adce8d Mon Sep 17 00:00:00 2001
From: Thom Lamb <thomlamb@gmail.com>
Date: Mon, 17 Aug 2026 11:07:44 -0500
Subject: [PATCH] fix(admin): confirm writes succeeded and allow inline
 category creation (#23)

Reported from testing: the item form said "Item added" for a save that
never happened.

saveItem and the other admin calls returned res.json() without checking
res.ok, so a 4xx/5xx resolved normally and every caller reported success
for a write the server had rejected. That is worse than failing outright,
because nothing prompts the user to look for the missing row. All admin
calls now throw on a non-OK response, and the handlers report the error,
keep the form open so entered values survive, and only claim success once
the server has accepted the write. Mark sold/available previously did
nothing visible on failure at all.

Categories can now be created from the item form, as tags already could.
Previously a category that did not exist yet meant abandoning a
half-filled form for the Categories tab. New categories are created at
the top level; nesting stays in the Categories tab.

The control lives in its own component: inline, every keystroke
re-rendered the whole Inventory component and rebuilt the category tree,
which visibly jittered the open popup. It sits above the tree rather than
below it, where a long list both hid it and made its position depend on
the list's measured height. The tree no longer expands everything on
open, which does not scale past a screenful; it has search instead.

The app now honours prefers-reduced-motion by disabling antd transitions,
and the e2e suite runs with that preference set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 frontend/playwright.config.ts                 |   7 +-
 frontend/src/admin/Admin.tsx                  |  73 +++++++-----
 frontend/src/admin/CategoryTreeSelect.tsx     | 107 ++++++++++++++++++
 frontend/src/api.ts                           |  34 +++++-
 frontend/src/main.tsx                         |  26 ++++-
 .../tests/e2e/admin-inline-category.spec.ts   |  77 +++++++++++++
 .../tests/e2e/admin-save-failures.spec.ts     |  75 ++++++++++++
 7 files changed, 361 insertions(+), 38 deletions(-)
 create mode 100644 frontend/src/admin/CategoryTreeSelect.tsx
 create mode 100644 frontend/tests/e2e/admin-inline-category.spec.ts
 create mode 100644 frontend/tests/e2e/admin-save-failures.spec.ts

diff --git a/frontend/playwright.config.ts b/frontend/playwright.config.ts
index 6ac5da9..f37abfa 100755
--- a/frontend/playwright.config.ts
+++ b/frontend/playwright.config.ts
@@ -7,7 +7,12 @@ export default defineConfig({
   reporter: [['list']],
   use: {
     baseURL: 'http://localhost:5173',
-    trace: 'on-first-retry'
+    trace: 'on-first-retry',
+    // The app turns off antd's transitions under this preference. Animated
+    // popups never settle long enough for Playwright's stability check when
+    // the machine is loaded, which showed up as clicks timing out on a button
+    // that was plainly visible and enabled.
+    reducedMotion: 'reduce'
   },
   webServer: {
     command: 'npm run dev',
diff --git a/frontend/src/admin/Admin.tsx b/frontend/src/admin/Admin.tsx
index c0cb761..57dcb9e 100755
--- a/frontend/src/admin/Admin.tsx
+++ b/frontend/src/admin/Admin.tsx
@@ -2,7 +2,7 @@ import { useEffect, useState } from 'react';
 import {
   Layout, Table, Button, Form, Input, InputNumber, Upload, Modal,
   Space, Tag, Typography, Switch, message, Image as AntImage, theme, Tabs,
-  TreeSelect, Select
+  Select
 } from 'antd';
 import { UploadOutlined, DeleteOutlined } from '@ant-design/icons';
 import type { UploadFile } from 'antd/es/upload/interface';
@@ -14,26 +14,12 @@ import {
   fetchAdminItems, saveItem, deleteItem, deleteItemImage, markSold, markAvailable,
   fetchAdminCategories, fetchAdminTags
 } from '../api';
-import { buildCategoryTree, CategoryNode } from '../filters';
 import { useThemeMode } from '../theme/ThemeContext';
 import Customers from './Customers';
 import Settings from './Settings';
 import Categories from './Categories';
 import Tags from './Tags';
-
-interface CategoryTreeOption {
-  value: number;
-  title: string;
-  children?: CategoryTreeOption[];
-}
-
-function toCategoryTreeData(nodes: CategoryNode[]): CategoryTreeOption[] {
-  return nodes.map(node => ({
-    value: node.id,
-    title: node.name,
-    children: node.children.length ? toCategoryTreeData(node.children) : undefined
-  }));
-}
+import CategoryTreeSelect from './CategoryTreeSelect';
 
 const { Header, Content } = Layout;
 const { Title } = Typography;
@@ -47,6 +33,7 @@ function Inventory() {
   const [description, setDescription] = useState<string>('');
   const [categories, setCategories] = useState<Category[]>([]);
   const [tags, setTags] = useState<TagRecord[]>([]);
+  const [saving, setSaving] = useState(false);
   const { mode } = useThemeMode();
 
   const load = () => fetchAdminItems().then(setItems);
@@ -94,21 +81,55 @@ function Inventory() {
     fd.append('category_id', values.category_id == null ? '' : String(values.category_id));
     fd.append('tags', JSON.stringify(values.tags ?? []));
     fileList.forEach(f => { if (f.originFileObj) fd.append('images', f.originFileObj as File); });
-    await saveItem(editingItem?.id ?? null, fd);
+
+    // Only report success, close the form, and discard the entered values once
+    // the server has actually accepted the write.
+    setSaving(true);
+    try {
+      await saveItem(editingItem?.id ?? null, fd);
+    } catch (err) {
+      message.error(`Couldn't save item — ${(err as Error).message}`);
+      return;
+    } finally {
+      setSaving(false);
+    }
+
     message.success(editingItem ? 'Item updated' : 'Item added');
     setModalOpen(false);
     load();
     loadOptions();
   }
 
+  // Status changes silently did nothing on failure — the row simply stayed put
+  // with no indication why.
+  async function handleStatusChange(action: (id: number) => Promise<Item>, id: number, label: string) {
+    try {
+      await action(id);
+    } catch (err) {
+      message.error(`Couldn't ${label} — ${(err as Error).message}`);
+      return;
+    }
+    load();
+  }
+
   async function handleDelete(id: number) {
-    await deleteItem(id);
+    try {
+      await deleteItem(id);
+    } catch (err) {
+      message.error(`Couldn't delete item — ${(err as Error).message}`);
+      return;
+    }
     message.success('Item deleted');
     load();
   }
 
   async function handleDeleteImage(itemId: number, imageId: number) {
-    await deleteItemImage(itemId, imageId);
+    try {
+      await deleteItemImage(itemId, imageId);
+    } catch (err) {
+      message.error(`Couldn't remove image — ${(err as Error).message}`);
+      return;
+    }
     message.success('Image removed');
     load();
     setEditingItem(prev => prev && prev.id === itemId
@@ -159,8 +180,8 @@ function Inventory() {
           <Button size="small" onClick={() => openEdit(item)}>Edit</Button>
           <Button size="small" danger onClick={() => handleDelete(item.id)}>Delete</Button>
           {item.status !== 'sold'
-            ? <Button size="small" onClick={() => markSold(item.id).then(load)}>Mark Sold</Button>
-            : <Button size="small" onClick={() => markAvailable(item.id).then(load)}>Mark Available</Button>}
+            ? <Button size="small" onClick={() => handleStatusChange(markSold, item.id, 'mark sold')}>Mark Sold</Button>
+            : <Button size="small" onClick={() => handleStatusChange(markAvailable, item.id, 'mark available')}>Mark Available</Button>}
         </Space>
       )
     }
@@ -174,7 +195,7 @@ function Inventory() {
       </div>
       <Table rowKey="id" dataSource={items} columns={columns} scroll={{ x: true }} />
 
-      <Modal title={editingItem ? 'Edit Item' : 'Add Item'} open={modalOpen} onOk={handleOk} onCancel={() => setModalOpen(false)} destroyOnClose width={720}>
+      <Modal title={editingItem ? 'Edit Item' : 'Add Item'} open={modalOpen} onOk={handleOk} confirmLoading={saving} onCancel={() => setModalOpen(false)} destroyOnHidden width={720}>
         <div data-color-mode={mode}>
           <Form form={form} layout="vertical">
             <Form.Item name="name" label="Name" rules={[{ required: true }]}>
@@ -187,13 +208,7 @@ function Inventory() {
               <InputNumber min={0} step={0.01} style={{ width: '100%' }} />
             </Form.Item>
             <Form.Item name="category_id" label="Category">
-              <TreeSelect
-                allowClear
-                placeholder="Uncategorized"
-                treeDefaultExpandAll
-                treeData={toCategoryTreeData(buildCategoryTree(categories))}
-                style={{ width: '100%' }}
-              />
+              <CategoryTreeSelect categories={categories} onCategoriesChanged={setCategories} />
             </Form.Item>
             <Form.Item
               name="tags"
diff --git a/frontend/src/admin/CategoryTreeSelect.tsx b/frontend/src/admin/CategoryTreeSelect.tsx
new file mode 100644
index 0000000..ca13b36
--- /dev/null
+++ b/frontend/src/admin/CategoryTreeSelect.tsx
@@ -0,0 +1,107 @@
+import { useMemo, useState } from 'react';
+import TreeSelect from 'antd/lib/tree-select';
+import Input from 'antd/lib/input';
+import Button from 'antd/lib/button';
+import Divider from 'antd/lib/divider';
+import message from 'antd/lib/message';
+import { Category, createCategory, fetchAdminCategories } from '../api';
+import { buildCategoryTree, CategoryNode } from '../filters';
+
+interface CategoryTreeOption {
+  value: number;
+  title: string;
+  children?: CategoryTreeOption[];
+}
+
+function toTreeData(nodes: CategoryNode[]): CategoryTreeOption[] {
+  return nodes.map((node) => ({
+    value: node.id,
+    title: node.name,
+    children: node.children.length ? toTreeData(node.children) : undefined
+  }));
+}
+
+interface Props {
+  // Supplied by antd's Form.Item. `id` has to be forwarded or the field loses
+  // its association with the rendered <label>, which breaks both screen readers
+  // and any lookup by label.
+  value?: number;
+  onChange?: (value: number | undefined) => void;
+  id?: string;
+  categories: Category[];
+  onCategoriesChanged: (categories: Category[]) => void;
+}
+
+// Pulled out of the item form so that typing a new category name re-renders
+// only this control. Left inline, every keystroke re-rendered the whole
+// Inventory component and rebuilt the tree data, which visibly jittered the
+// open popup and moved the buttons under the pointer.
+export default function CategoryTreeSelect({ value, onChange, id, categories, onCategoriesChanged }: Props) {
+  const [newCategoryName, setNewCategoryName] = useState('');
+  const [creating, setCreating] = useState(false);
+
+  const treeData = useMemo(() => toTreeData(buildCategoryTree(categories)), [categories]);
+
+  // Tags can be invented from the item form, so categories should be too —
+  // otherwise adding an item in a new category means abandoning a half-filled
+  // form. New categories land at the top level; nesting is done in the
+  // Categories tab, keeping this control to a single decision.
+  async function handleCreate() {
+    const name = newCategoryName.trim();
+    if (!name) return;
+
+    setCreating(true);
+    try {
+      const created = await createCategory(name, null);
+      onCategoriesChanged(await fetchAdminCategories());
+      onChange?.(created.id);
+      setNewCategoryName('');
+      message.success(`Category "${created.name}" added`);
+    } catch (err) {
+      message.error(`Couldn't create category — ${(err as Error).message}`);
+    } finally {
+      setCreating(false);
+    }
+  }
+
+  return (
+    <TreeSelect
+      allowClear
+      showSearch
+      placeholder="Uncategorized"
+      // Deliberately not treeDefaultExpandAll: expanding a large tree on open
+      // makes the popup reflow while it measures, and buries the create field
+      // below every row. Search is the better affordance past a screenful.
+      treeNodeFilterProp="title"
+      listHeight={256}
+      treeData={treeData}
+      id={id}
+      value={value}
+      onChange={onChange}
+      style={{ width: '100%' }}
+      popupRender={(menu) => (
+        <>
+          {/* Above the tree, not below it. Under a long list the control is
+              both invisible without scrolling and positioned by the list's
+              measured height, so it shifts as the virtualized rows settle. */}
+          <div style={{ display: 'flex', gap: 8, padding: '8px 8px 0' }}>
+            <Input
+              placeholder="New category name"
+              value={newCategoryName}
+              onChange={(event) => setNewCategoryName(event.target.value)}
+              // Without this the tree steals the keystrokes for its own
+              // type-ahead and arrow-key navigation.
+              onKeyDown={(event) => event.stopPropagation()}
+              onPressEnter={handleCreate}
+            />
+            <Button type="primary" loading={creating} onClick={handleCreate}>
+              Create category
+            </Button>
+          </div>
+          <Divider style={{ margin: '8px 0' }} />
+          {menu}
+        </>
+      )}
+    />
+  );
+}
diff --git a/frontend/src/api.ts b/frontend/src/api.ts
index 0dfddb6..25879e2 100755
--- a/frontend/src/api.ts
+++ b/frontend/src/api.ts
@@ -67,32 +67,54 @@ export async function fetchFilterOptions(): Promise<FilterOptions> {
   return res.json();
 }
 
+// Every admin call goes through this. Without the res.ok check a 4xx/5xx still
+// resolves — the caller then reports success for a write that never happened,
+// which is worse than failing outright because nothing prompts the user to look
+// for the missing row.
+async function expectOk(res: Response, action: string): Promise<Response> {
+  if (res.ok) return res;
+  const detail = await res.json().catch(() => null);
+  throw new Error(detail?.error ? `${action}: ${detail.error}` : action);
+}
+
 export async function fetchAdminItems(): Promise<Item[]> {
-  const res = await fetch('/api/admin/items');
+  const res = await expectOk(await fetch('/api/admin/items'), 'failed to load items');
   return res.json();
 }
 
 export async function saveItem(id: number | null, formData: FormData): Promise<Item> {
   const url = id ? `/api/admin/items/${id}` : '/api/admin/items';
-  const res = await fetch(url, { method: id ? 'PUT' : 'POST', body: formData });
+  const res = await expectOk(
+    await fetch(url, { method: id ? 'PUT' : 'POST', body: formData }),
+    'failed to save item'
+  );
   return res.json();
 }
 
 export async function deleteItem(id: number): Promise<void> {
-  await fetch(`/api/admin/items/${id}`, { method: 'DELETE' });
+  await expectOk(await fetch(`/api/admin/items/${id}`, { method: 'DELETE' }), 'failed to delete item');
 }
 
 export async function deleteItemImage(itemId: number, imageId: number): Promise<void> {
-  await fetch(`/api/admin/items/${itemId}/images/${imageId}`, { method: 'DELETE' });
+  await expectOk(
+    await fetch(`/api/admin/items/${itemId}/images/${imageId}`, { method: 'DELETE' }),
+    'failed to remove image'
+  );
 }
 
 export async function markSold(id: number): Promise<Item> {
-  const res = await fetch(`/api/admin/items/${id}/mark-sold`, { method: 'POST' });
+  const res = await expectOk(
+    await fetch(`/api/admin/items/${id}/mark-sold`, { method: 'POST' }),
+    'failed to mark sold'
+  );
   return res.json();
 }
 
 export async function markAvailable(id: number): Promise<Item> {
-  const res = await fetch(`/api/admin/items/${id}/mark-available`, { method: 'POST' });
+  const res = await expectOk(
+    await fetch(`/api/admin/items/${id}/mark-available`, { method: 'POST' }),
+    'failed to mark available'
+  );
   return res.json();
 }
 
diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx
index ce7ba83..df13c10 100755
--- a/frontend/src/main.tsx
+++ b/frontend/src/main.tsx
@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { useEffect, useState } from 'react';
 import ReactDOM from 'react-dom/client';
 import { BrowserRouter, Routes, Route } from 'react-router-dom';
 import { ConfigProvider, theme as antdTheme } from 'antd';
@@ -16,13 +16,35 @@ import { CartProvider } from './cart/CartContext';
 import { ThemeModeProvider, useThemeMode } from './theme/ThemeContext';
 import './styles.css';
 
+const REDUCED_MOTION_QUERY = '(prefers-reduced-motion: reduce)';
+
+// Respects the OS-level "reduce motion" accessibility setting by turning off
+// antd's transitions. Beyond the accessibility win, animated popups are a
+// standing source of flake in end-to-end tests, which drive the app with this
+// preference enabled.
+function usePrefersReducedMotion(): boolean {
+  const [prefers, setPrefers] = useState(
+    () => typeof window !== 'undefined' && window.matchMedia(REDUCED_MOTION_QUERY).matches
+  );
+
+  useEffect(() => {
+    const query = window.matchMedia(REDUCED_MOTION_QUERY);
+    const update = () => setPrefers(query.matches);
+    query.addEventListener('change', update);
+    return () => query.removeEventListener('change', update);
+  }, []);
+
+  return prefers;
+}
+
 function Root() {
   const { mode } = useThemeMode();
+  const prefersReducedMotion = usePrefersReducedMotion();
   return (
     <ConfigProvider
       theme={{
         algorithm: mode === 'dark' ? antdTheme.darkAlgorithm : antdTheme.defaultAlgorithm,
-        token: { colorPrimary: '#1a1a1a' }
+        token: { colorPrimary: '#1a1a1a', motion: !prefersReducedMotion }
       }}
     >
       <BrowserRouter>
diff --git a/frontend/tests/e2e/admin-inline-category.spec.ts b/frontend/tests/e2e/admin-inline-category.spec.ts
new file mode 100644
index 0000000..f639fe3
--- /dev/null
+++ b/frontend/tests/e2e/admin-inline-category.spec.ts
@@ -0,0 +1,77 @@
+import { test, expect } from '@playwright/test';
+
+const suffix = () => `i${Date.now().toString(36)}${Math.random().toString(36).slice(2, 7)}`;
+
+// Opening the form kicks off a categories fetch. Interacting with the category
+// field before it lands means the tree re-renders under the cursor, so wait for
+// the data rather than racing it.
+async function openItemForm(page: import('@playwright/test').Page) {
+  // Opening the form fetches both categories and tags, and each one re-renders
+  // the modal as it lands. Waiting for only one still leaves the second to
+  // reflow the popup mid-interaction.
+  const loaded = Promise.all([
+    page.waitForResponse((res) => res.url().includes('/api/admin/categories') && res.request().method() === 'GET'),
+    page.waitForResponse((res) => res.url().includes('/api/admin/tags') && res.request().method() === 'GET')
+  ]);
+  await page.getByRole('button', { name: 'Add Item' }).click();
+  await loaded;
+}
+
+test.describe('Inline category creation from the item form', () => {
+  test('creates a category without leaving the item form and assigns it', async ({ page }) => {
+    const RUN = suffix();
+    const categoryName = `Inline ${RUN}`;
+    const itemName = `Item ${RUN}`;
+
+    await page.goto('/admin');
+    await openItemForm(page);
+    await page.getByLabel('Name').fill(itemName);
+    await page.getByLabel('Price (USD)').fill('99');
+
+    await page.getByRole('dialog').getByLabel('Category', { exact: true }).click();
+    const nameInput = page.getByPlaceholder('New category name');
+    await expect(nameInput).toBeVisible();
+    await expect(page.getByRole('button', { name: 'Create category' })).toBeVisible();
+    await nameInput.fill(categoryName);
+    // Submitted with Enter rather than a click: the popup sits over a
+    // virtualized tree that keeps re-measuring, so a click target inside it is
+    // never geometrically stable. Enter runs the same handler as the button.
+    await nameInput.press('Enter');
+
+    // The new category should be selected straight away — having to hunt for it
+    // in the tree afterwards defeats the point of creating it inline.
+    await expect(page.getByRole('dialog').getByText(categoryName)).toBeVisible();
+
+    await page.getByRole('button', { name: 'OK' }).click();
+    await expect(page.getByText('Item added')).toBeVisible();
+
+    const items = await (await page.request.get('/api/admin/items')).json();
+    const saved = items.find((item: { name: string }) => item.name === itemName);
+    expect(saved).toBeTruthy();
+    expect(saved.category_name).toBe(categoryName);
+  });
+
+  test('reports a duplicate category name instead of silently doing nothing', async ({ page }) => {
+    const RUN = suffix();
+    const categoryName = `Dupe ${RUN}`;
+
+    const created = await page.request.post('/api/admin/categories', {
+      data: { name: categoryName, parent_id: null }
+    });
+    expect(created.status()).toBe(201);
+
+    await page.goto('/admin');
+    await openItemForm(page);
+    await page.getByRole('dialog').getByLabel('Category', { exact: true }).click();
+    const nameInput = page.getByPlaceholder('New category name');
+    await expect(nameInput).toBeVisible();
+    await expect(page.getByRole('button', { name: 'Create category' })).toBeVisible();
+    await nameInput.fill(categoryName);
+    // Submitted with Enter rather than a click: the popup sits over a
+    // virtualized tree that keeps re-measuring, so a click target inside it is
+    // never geometrically stable. Enter runs the same handler as the button.
+    await nameInput.press('Enter');
+
+    await expect(page.getByText(/already exists/i)).toBeVisible();
+  });
+});
diff --git a/frontend/tests/e2e/admin-save-failures.spec.ts b/frontend/tests/e2e/admin-save-failures.spec.ts
new file mode 100644
index 0000000..0476d6d
--- /dev/null
+++ b/frontend/tests/e2e/admin-save-failures.spec.ts
@@ -0,0 +1,75 @@
+import { test, expect } from '@playwright/test';
+
+const suffix = () => `s${Date.now().toString(36)}${Math.random().toString(36).slice(2, 7)}`;
+
+test.describe('Admin save failures', () => {
+  test('does not claim an item was saved when the request failed', async ({ page }) => {
+    await page.route('**/api/admin/items', (route) => {
+      if (route.request().method() === 'POST') {
+        return route.fulfill({
+          status: 500,
+          contentType: 'application/json',
+          body: '{"error":"internal error"}'
+        });
+      }
+      return route.continue();
+    });
+
+    await page.goto('/admin');
+    await page.getByRole('button', { name: 'Add Item' }).click();
+    await page.getByLabel('Name').fill(`Broken ${suffix()}`);
+    await page.getByLabel('Price (USD)').fill('12');
+    await page.getByRole('button', { name: 'OK' }).click();
+
+    // Reporting success for a failed save is worse than failing loudly: the
+    // item is silently absent and the user has no reason to look for it.
+    await expect(page.getByText('Item added')).toBeHidden();
+    await expect(page.getByText("Couldn't save item")).toBeVisible();
+    // The form must stay open so the entered values aren't lost.
+    await expect(page.getByRole('dialog')).toBeVisible();
+  });
+
+  test('reports a failed delete rather than claiming success', async ({ page }) => {
+    // Seeded through the API so the test owns a known row rather than clicking
+    // whichever Delete button happens to be first in a paginated table.
+    const name = `Doomed ${suffix()}`;
+    const created = await page.request.post('/api/admin/items', {
+      multipart: { name, description: '', price: '10', category_id: '', tags: '[]' }
+    });
+    expect(created.ok()).toBeTruthy();
+
+    await page.route('**/api/admin/items/*', (route) => {
+      if (route.request().method() === 'DELETE') {
+        return route.fulfill({ status: 500, contentType: 'application/json', body: '{"error":"nope"}' });
+      }
+      return route.continue();
+    });
+
+    await page.goto('/admin');
+    // Items list newest-first, so the seeded row is on the first page.
+    const row = page.getByRole('row').filter({ hasText: name });
+    await expect(row).toBeVisible();
+    await row.getByRole('button', { name: 'Delete' }).click();
+
+    await expect(page.getByText('Item deleted')).toBeHidden();
+    await expect(page.getByText("Couldn't delete item")).toBeVisible();
+    // The row must survive a failed delete.
+    await expect(row).toBeVisible();
+  });
+
+  test('saves an item successfully when the server accepts it', async ({ page }) => {
+    const name = `Good ${suffix()}`;
+
+    await page.goto('/admin');
+    await page.getByRole('button', { name: 'Add Item' }).click();
+    await page.getByLabel('Name').fill(name);
+    await page.getByLabel('Price (USD)').fill('34');
+    await page.getByRole('button', { name: 'OK' }).click();
+
+    await expect(page.getByText('Item added')).toBeVisible();
+
+    // Confirm the row actually reached the database, not just that a toast fired.
+    const items = await (await page.request.get('/api/admin/items')).json();
+    expect(items.some((item: { name: string }) => item.name === name)).toBeTruthy();
+  });
+});