From 8898213e021ac64f85b02bfb91d438a9309610a9 Mon Sep 17 00:00:00 2001 From: le king fu Date: Mon, 12 Oct 2026 18:44:54 -0400 Subject: [PATCH] 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 --- .../categories/AddKeywordDialog.tsx | 9 +- .../CategoryDetailPanel.render.test.tsx | 84 +++++++++++++++++++ .../categories/CategoryForm.render.test.tsx | 19 +++++ .../categories/parentOptions.test.ts | 7 ++ src/components/categories/parentOptions.ts | 15 ++-- 5 files changed, 123 insertions(+), 11 deletions(-) create mode 100644 src/components/categories/CategoryDetailPanel.render.test.tsx diff --git a/src/components/categories/AddKeywordDialog.tsx b/src/components/categories/AddKeywordDialog.tsx index e593c06..69c12ee 100644 --- a/src/components/categories/AddKeywordDialog.tsx +++ b/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(null); const [categories, setCategories] = useState([]); - const [categoriesLoaded, setCategoriesLoaded] = useState(false); const [checked, setChecked] = useState>(new Set()); const [applyToHidden, setApplyToHidden] = useState(false); const [preview, setPreview] = useState({ 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 && diff --git a/src/components/categories/CategoryDetailPanel.render.test.tsx b/src/components/categories/CategoryDetailPanel.render.test.tsx new file mode 100644 index 0000000..434b8f0 --- /dev/null +++ b/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) => + 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(); + 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( + ({ 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 }); + }); +}); diff --git a/src/components/categories/CategoryForm.render.test.tsx b/src/components/categories/CategoryForm.render.test.tsx index fe51cc2..22ba643 100644 --- a/src/components/categories/CategoryForm.render.test.tsx +++ b/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(); + 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 }); + }); }); diff --git a/src/components/categories/parentOptions.test.ts b/src/components/categories/parentOptions.test.ts index dd2cc64..def71fa 100644 --- a/src/components/categories/parentOptions.test.ts +++ b/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]); + }); }); diff --git a/src/components/categories/parentOptions.ts b/src/components/categories/parentOptions.ts index 52f19f9..04fd296 100644 --- a/src/components/categories/parentOptions.ts +++ b/src/components/categories/parentOptions.ts @@ -36,7 +36,8 @@ function depthsOf(categories: CategoryTreeNode[]): Map { /** * 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(); @@ -49,7 +50,7 @@ function subtreeHeight(categories: CategoryTreeNode[], rootId: number): number { let height = 0; const seen = new Set([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; });