refactor(categories): follow-ups of the review of PR #428

Refs #423

- parentOptions.ts: say that this list is the only guard today (updateCategory
  checks neither depth nor cycle, #393 will), instead of pointing to a
  service-side validation that does not exist yet. The height walk is also
  bounded by the number of categories, so a missing cycle guard gives a wrong
  height instead of an endless loop; a test edits a member of a cycle.
- AddKeywordDialog.tsx: drop `categoriesLoaded`, which only repeated what the
  null initial category already guarantees.
- Tests: the form and the detail panel are pinned to what they hand to
  buildParentOptions (editingId, currentParentId), through a pass-through spy.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

src/components/categories/AddKeywordDialog.tsx

@@ -33,11 +33,11 @@ export default function AddKeywordDialog({
 }: AddKeywordDialogProps) {
   const { t } = useTranslation();
   const [keyword, setKeyword] = useState(initialKeyword);
-  // Held at null until the list is in: the pre-filled category is only kept
-  // when it is among the categories the dialog offers (`reconcileInitialCategory`).
+  // Held at null until the list is in, which keeps the apply button disabled
+  // meanwhile: the pre-filled category is only kept when it is among the
+  // categories the dialog offers (`reconcileInitialCategory`).
   const [categoryId, setCategoryId] = useState<number | null>(null);
   const [categories, setCategories] = useState<Category[]>([]);
-  const [categoriesLoaded, setCategoriesLoaded] = useState(false);
   const [checked, setChecked] = useState<Set<number>>(new Set());
   const [applyToHidden, setApplyToHidden] = useState(false);
   const [preview, setPreview] = useState<PreviewState>({ kind: "idle" });
@@ -55,13 +55,11 @@ export default function AddKeywordDialog({
         if (cancelled) return;
         setCategories(rows);
         setCategoryId(reconcileInitialCategory(initialCategoryId, rows));
-        setCategoriesLoaded(true);
       })
       .catch(() => {
         if (cancelled) return;
         setCategories([]);
         setCategoryId(null);
-        setCategoriesLoaded(true);
       });
     return () => {
       cancelled = true;
@@ -101,7 +99,6 @@ export default function AddKeywordDialog({
 
   const canApply =
     validation.ok &&
-    categoriesLoaded &&
     categoryId !== null &&
     preview.kind === "ready" &&
     !isApplying &&

src/components/categories/CategoryDetailPanel.render.test.tsx

@@ -0,0 +1,84 @@
+// CategoryDetailPanel — the edited category's id reaches the parent options (#423).
+// No jsdom: react-dom/server renders in vitest's node environment.
+import { describe, it, expect, vi } from "vitest";
+import { renderToStaticMarkup } from "react-dom/server";
+
+vi.mock("react-i18next", () => ({
+  useTranslation: () => ({
+    t: (k: string, o?: Record<string, unknown>) =>
+      o ? `${k}[${Object.entries(o).map(([a, b]) => `${a}=${String(b)}`).join(",")}]` : k,
+    i18n: { language: "fr" },
+  }),
+}));
+
+vi.mock("./parentOptions", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("./parentOptions")>();
+  return { ...actual, buildParentOptions: vi.fn(actual.buildParentOptions) };
+});
+
+import CategoryDetailPanel from "./CategoryDetailPanel";
+import { buildParentOptions } from "./parentOptions";
+import type { CategoryFormData, CategoryTreeNode } from "../../shared/types";
+
+const node = (id: number, parent_id: number | null, name: string): CategoryTreeNode => ({
+  id,
+  name,
+  parent_id,
+  color: null,
+  icon: null,
+  type: "expense",
+  is_active: true,
+  is_inputable: true,
+  sort_order: id,
+  keyword_count: 0,
+  i18n_key: null,
+  children: [],
+});
+
+const HOUSING = node(1, null, "Logement");
+const RENT = node(10, 1, "Loyer");
+const CATEGORIES = [HOUSING, RENT];
+
+const formOf = (c: CategoryTreeNode): CategoryFormData => ({
+  name: c.name,
+  type: c.type,
+  color: "#4A90A4",
+  parent_id: c.parent_id,
+  is_inputable: c.is_inputable,
+  sort_order: c.sort_order,
+});
+
+const markup = (selected: CategoryTreeNode | null, editing: CategoryFormData | null, isCreating: boolean) =>
+  renderToStaticMarkup(
+    <CategoryDetailPanel
+      categories={CATEGORIES}
+      selectedCategory={selected}
+      keywords={[]}
+      editingCategory={editing}
+      isCreating={isCreating}
+      isSaving={false}
+      onStartEditing={vi.fn()}
+      onCancelEditing={vi.fn()}
+      onSave={vi.fn()}
+      onDelete={vi.fn(async () => ({ blocked: false, count: 0 }))}
+      onAddKeyword={vi.fn()}
+      onUpdateKeyword={vi.fn()}
+      onRemoveKeyword={vi.fn()}
+    />,
+  );
+
+describe("CategoryDetailPanel — editingId wiring (#423)", () => {
+  it("passes the selected category's id when editing", () => {
+    vi.mocked(buildParentOptions).mockClear();
+    const html = markup(RENT, formOf(RENT), false);
+    expect(html).toContain('role="combobox"');
+    expect(buildParentOptions).toHaveBeenCalledWith(CATEGORIES, { editingId: 10, currentParentId: 1 });
+  });
+
+  it("passes no id when creating", () => {
+    vi.mocked(buildParentOptions).mockClear();
+    const html = markup(null, formOf(node(0, null, "")), true);
+    expect(html).toContain('role="combobox"');
+    expect(buildParentOptions).toHaveBeenCalledWith(CATEGORIES, { editingId: null, currentParentId: null });
+  });
+});

src/components/categories/CategoryForm.render.test.tsx

@@ -12,7 +12,14 @@ vi.mock("react-i18next", () => ({
   }),
 }));
 
+// Pass-through spy on the pure module, to pin what the form hands it.
+vi.mock("./parentOptions", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("./parentOptions")>();
+  return { ...actual, buildParentOptions: vi.fn(actual.buildParentOptions) };
+});
+
 import CategoryForm from "./CategoryForm";
+import { buildParentOptions } from "./parentOptions";
 import type { CategoryFormData, CategoryTreeNode } from "../../shared/types";
 
 const node = (
@@ -92,4 +99,16 @@ describe("CategoryForm — parent field (#423)", () => {
   it("shows a user-created parent by its name", () => {
     expect(comboboxInput(markup(form(10), 42))).toContain('value="Loyer"');
   });
+
+  it("hands the edited id and the stored parent to buildParentOptions", () => {
+    vi.mocked(buildParentOptions).mockClear();
+    markup(form(1), 10);
+    expect(buildParentOptions).toHaveBeenCalledWith(CATEGORIES, { editingId: 10, currentParentId: 1 });
+  });
+
+  it("hands no edited id when creating", () => {
+    vi.mocked(buildParentOptions).mockClear();
+    markup(form(null));
+    expect(buildParentOptions).toHaveBeenCalledWith(CATEGORIES, { editingId: null, currentParentId: null });
+  });
 });

src/components/categories/parentOptions.test.ts

@@ -96,4 +96,11 @@ describe("buildParentOptions — editing a category", () => {
     expect(offered).not.toContain(3);
     expect(offered.every((id) => id === 1 || id === 2)).toBe(true);
   });
+
+  it("terminates when the edited category is itself a member of a cycle", () => {
+    // 1 <-> 2: the height walk under 1 meets 1 again and stops (height 1, not
+    // unbounded). Only roots remain, so the root outside the cycle is offered.
+    const cyclic = [node(1, 2), node(2, 1), node(3, null)];
+    expect(ids(buildParentOptions(cyclic, { editingId: 1 }))).toEqual([3]);
+  });
 });

src/components/categories/parentOptions.ts

@@ -36,7 +36,8 @@ function depthsOf(categories: CategoryTreeNode[]): Map<number, number> {
 
 /**
  * Height of the sub-tree under `rootId`: 0 for a leaf, 1 with children, 2 with
- * grandchildren. Walks `parent_id` links; a cycle is not walked twice.
+ * grandchildren. Walks `parent_id` links; a cycle is not walked twice, and the
+ * walk can never take more steps than there are categories.
  */
 function subtreeHeight(categories: CategoryTreeNode[], rootId: number): number {
   const childrenOf = new Map<number, number[]>();
@@ -49,7 +50,7 @@ function subtreeHeight(categories: CategoryTreeNode[], rootId: number): number {
   let height = 0;
   const seen = new Set<number>([rootId]);
   let frontier = [rootId];
-  while (frontier.length > 0) {
+  while (frontier.length > 0 && height < categories.length) {
     const next: number[] = [];
     for (const id of frontier) {
       for (const child of childrenOf.get(id) ?? []) {
@@ -79,6 +80,10 @@ function subtreeHeight(categories: CategoryTreeNode[], rootId: number): number {
  *   deeper than any depth still allowed.
  * - The parent currently set on the form is always kept when it exists.
  *
+ * This list is the only guard: `updateCategory` writes any `parent_id` it is
+ * given, without a depth or cycle check (#393 will add one). The form therefore
+ * never offers a new parent that the rules above reject.
+ *
  * Ordering is left to the caller (`CategoryCombobox` sorts hierarchically).
  */
 export function buildParentOptions(
@@ -95,9 +100,9 @@ export function buildParentOptions(
 
   return categories.filter((c) => {
     if (c.id === editingId) return false;
-    // Kept even when the depth rule or the descent rule would drop it: the
-    // field must show what is stored. Saving an invalid parent is refused by
-    // the service-side validation of #393, not here.
+    // Kept even when the depth rule would drop it: the field must show what is
+    // stored. Keeping it changes nothing on save (it is already the parent);
+    // repairing such data is the job of #393.
     if (c.id === currentParentId) return true;
     return (depths.get(c.id) ?? 0) <= maxDepth;
   });