fix(categories): shrink the allowed parent depth by the height of the edited sub-tree
All checks were successful
PR Check — Frontend / frontend (pull_request) Successful in 1m54s

Refs #423

The review of PR #428 showed that the rule only looked at direct children:
editing a root that carries grandchildren still offered the other roots as
a parent, and saving created a fourth level, which nothing guards against
in updateCategory or moveCategory. The allowed parent depth is now
MAX_PARENT_DEPTH minus the height of the edited sub-tree (0, 1 or 2), so a
category with grandchildren gets no parent at all, only its current one.

Test written first on the reviewer's probe ([1, 10->1, 100->10, 2],
editing 1: [] expected, [2] obtained before the fix).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
le king fu 2026-10-12 18:15:52 -04:00
parent 4bdb00402e
commit 04ac2e933e
2 changed files with 55 additions and 15 deletions

View file

@ -47,17 +47,24 @@ describe("buildParentOptions — editing a category", () => {
expect(ids(buildParentOptions(TREE, { editingId: 11 }))).toEqual([1, 2, 10, 20]); expect(ids(buildParentOptions(TREE, { editingId: 11 }))).toEqual([1, 2, 10, 20]);
}); });
it("leaves out the whole descent of the edited category", () => { it("offers nothing when the edited category has grandchildren", () => {
// Editing root 1, which has children: only roots remain, so none of its // Root 1 carries 10 -> 100: under any parent, 100 would sit at level 3.
// descent (10, 11, 100) can be offered, whatever its depth. // Probe of the review of PR #428: `[1, 10->1, 100->10, 2]`, editing 1.
expect(ids(buildParentOptions(TREE, { editingId: 1 }))).toEqual([2]); expect(ids(buildParentOptions([node(1, null), node(10, 1), node(100, 10), node(2, null)], { editingId: 1 }))).toEqual([]);
// Editing 10 (child 100): a level-1 category with a child; 100 stays out. expect(ids(buildParentOptions(TREE, { editingId: 1 }))).toEqual([]);
expect(ids(buildParentOptions(TREE, { editingId: 10 }))).not.toContain(100);
}); });
it("keeps only roots when the edited category has children", () => { it("leaves out the whole descent of the edited category", () => {
// Editing 10 (child 100): a level-1 category with a child; 100 stays out.
expect(ids(buildParentOptions(TREE, { editingId: 10 }))).not.toContain(100);
// Editing root 2 (child 20): only roots remain, so 20 stays out.
expect(ids(buildParentOptions(TREE, { editingId: 2 }))).not.toContain(20);
});
it("keeps only roots when the edited category has children but no grandchildren", () => {
// 10 has a child (100): under a level-1 parent its child would sit at level 3. // 10 has a child (100): under a level-1 parent its child would sit at level 3.
expect(ids(buildParentOptions(TREE, { editingId: 10 }))).toEqual([1, 2]); expect(ids(buildParentOptions(TREE, { editingId: 10 }))).toEqual([1, 2]);
expect(ids(buildParentOptions(TREE, { editingId: 2 }))).toEqual([1]);
}); });
it("keeps level-1 categories when the edited category is a leaf", () => { it("keeps level-1 categories when the edited category is a leaf", () => {

View file

@ -34,16 +34,49 @@ function depthsOf(categories: CategoryTreeNode[]): Map<number, number> {
return depths; return depths;
} }
/**
* 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.
*/
function subtreeHeight(categories: CategoryTreeNode[], rootId: number): number {
const childrenOf = new Map<number, number[]>();
for (const c of categories) {
if (c.parent_id === null) continue;
const bucket = childrenOf.get(c.parent_id) ?? [];
bucket.push(c.id);
childrenOf.set(c.parent_id, bucket);
}
let height = 0;
const seen = new Set<number>([rootId]);
let frontier = [rootId];
while (frontier.length > 0) {
const next: number[] = [];
for (const id of frontier) {
for (const child of childrenOf.get(id) ?? []) {
if (seen.has(child)) continue;
seen.add(child);
next.push(child);
}
}
if (next.length === 0) break;
height += 1;
frontier = next;
}
return height;
}
/** /**
* Categories that may be chosen as the parent in the category form. * Categories that may be chosen as the parent in the category form.
* *
* - Roots and level-1 categories only: a level-2 parent would create a fourth * - Roots and level-1 categories only: a level-2 parent would create a fourth
* level, which nothing else in the app knows how to display. * level, which nothing else in the app knows how to display.
* - When editing, the edited category is left out, and when it has children of * - When editing, the edited category is left out, and the allowed parent depth
* its own, only roots remain (its children would otherwise land at level 3). * shrinks by the height of its sub-tree: with children, only roots remain
* This alone also leaves out its whole descent, which would create a cycle * (the children would otherwise land at level 3); with grandchildren, no
* that hides the branch everywhere (#393): a category with descendants has * parent at all (they would land at level 4). This alone also leaves out its
* children, so only roots are offered, and a descendant is never a root. * whole descent, which would create a cycle that hides the branch everywhere
* (#393): a descendant sits at least one level below the edited category,
* deeper than any depth still allowed.
* - The parent currently set on the form is always kept when it exists. * - The parent currently set on the form is always kept when it exists.
* *
* Ordering is left to the caller (`CategoryCombobox` sorts hierarchically). * Ordering is left to the caller (`CategoryCombobox` sorts hierarchically).
@ -56,9 +89,9 @@ export function buildParentOptions(
const currentParentId = ctx.currentParentId ?? null; const currentParentId = ctx.currentParentId ?? null;
const depths = depthsOf(categories); const depths = depthsOf(categories);
const editedHasChildren = const height = editingId !== null ? subtreeHeight(categories, editingId) : 0;
editingId !== null && categories.some((c) => c.parent_id === editingId); // Negative when the edited category has grandchildren: only the current parent survives.
const maxDepth = editedHasChildren ? 0 : MAX_PARENT_DEPTH; const maxDepth = MAX_PARENT_DEPTH - height;
return categories.filter((c) => { return categories.filter((c) => {
if (c.id === editingId) return false; if (c.id === editingId) return false;