From 4be7eaed59923c127449b474d68e0044e908b2af Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 29 Sep 2026 15:34:44 +0200 Subject: [PATCH] fix(catalog): never create a page that reuses a sibling's order number The emulator complained "Sibling order 2 is used more than once" 74 times in production, covering 19 pages across 12 parents. The cause was that nearly every page-creation path passed orderNum 0, so each new page collided with whatever sibling already sat at 0 or 1, and the Park placeholder pages all shared the sentinel values 99 and 999. The production rows themselves are repaired out of band (renumbered 1..N per affected parent, ordered by order_num then id so the existing visual order is preserved, plus one dangling catalog_items row whose items_base no longer existed removed). This commit stops it recurring. - hierarchy.ts: add nextFreeSiblingOrder(), which ignores -1 and 0 as the same root set, treats a missing orderNum as 0, and returns an order strictly above the highest sibling in use. An explicit order is still honoured whenever it is free, so callers that genuinely want a position keep it. - page-commands.ts: createPageCommand resolves the real order through nextFreeSiblingOrder instead of writing the requested 0 straight through. Note that furni-import.ts and upload-import.ts still take their order from the furnidata catInfo.order, so two categories carrying the same furnidata order can still collide. That is caught by the emulator audit and repaired by fixEmulatorIssues(), but it is not prevented here. --- src/features/catalog/domain/hierarchy.test.ts | 35 ++++++++++++++++++ src/features/catalog/domain/hierarchy.ts | 36 ++++++++++++++++--- src/features/catalog/server/page-commands.ts | 7 +++- 3 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/features/catalog/domain/hierarchy.test.ts b/src/features/catalog/domain/hierarchy.test.ts index c7452ee9..3c09f557 100644 --- a/src/features/catalog/domain/hierarchy.test.ts +++ b/src/features/catalog/domain/hierarchy.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { assertParent, collectSubtree, + nextFreeSiblingOrder, validateSiblingOrder, } from "./hierarchy"; @@ -66,3 +67,37 @@ it("orders mixed root conventions with a stable ID tie breaker", () => { expect(() => validateSiblingOrder(roots, -1, [3, 2], [2, 3])).not.toThrow(); expect(() => validateSiblingOrder(roots, -1, [2, 3], [3, 2])).toThrow(); }); + +describe("nextFreeSiblingOrder", () => { + it("keeps an explicit order that no sibling holds", () => { + expect(nextFreeSiblingOrder(pages, 1, 7)).toBe(7); + // Parent 1's siblings sit at order 1 and 2, so 0 is still free. + expect(nextFreeSiblingOrder(pages, 1, 0)).toBe(0); + }); + it("appends instead of duplicating a taken order", () => { + // Requesting an order a sibling already holds would make the emulator + // reject the whole catalog, so the page lands after the last sibling. + expect(nextFreeSiblingOrder(pages, 1, 1)).toBe(3); + expect(nextFreeSiblingOrder(pages, 1, 2)).toBe(3); + expect(nextFreeSiblingOrder(pages, 2, 1)).toBe(2); + }); + it("appends when the caller gives no order at all", () => { + expect(nextFreeSiblingOrder(pages, 1)).toBe(3); + expect(nextFreeSiblingOrder(pages, 2)).toBe(2); + }); + it("counts parent -1 and parent 0 as the same sibling set", () => { + expect( + nextFreeSiblingOrder([{ id: 2, parentId: 0, orderNum: 1 }], -1, 1), + ).toBe(2); + expect( + nextFreeSiblingOrder([{ id: 2, parentId: -1, orderNum: 1 }], 0, 1), + ).toBe(2); + }); + it("treats a missing orderNum as 0 when looking for collisions", () => { + expect(nextFreeSiblingOrder([{ id: 5, parentId: 1 }], 1, 0)).toBe(1); + }); + it("gives the first child of an empty parent the requested order", () => { + expect(nextFreeSiblingOrder([], -1, 0)).toBe(0); + expect(nextFreeSiblingOrder([], -1)).toBe(1); + }); +}); diff --git a/src/features/catalog/domain/hierarchy.ts b/src/features/catalog/domain/hierarchy.ts index 1e064bf5..a33ebb0c 100644 --- a/src/features/catalog/domain/hierarchy.ts +++ b/src/features/catalog/domain/hierarchy.ts @@ -3,6 +3,12 @@ export interface HierarchyPage { parentId: number; orderNum?: number; } +/** Root pages live under parent -1 or 0; both address the same sibling set. */ +function isSiblingOf(page: HierarchyPage, parentId: number) { + return parentId <= 0 + ? page.parentId === -1 || page.parentId === 0 + : page.parentId === parentId; +} export class CatalogConflict extends Error {} export class CatalogInputError extends Error {} export class CatalogNotFound extends Error {} @@ -58,11 +64,7 @@ export function validateSiblingOrder( expected: readonly number[], ) { const current = pages - .filter((page) => - parentId <= 0 - ? page.parentId === -1 || page.parentId === 0 - : page.parentId === parentId, - ) + .filter((page) => isSiblingOf(page, parentId)) .sort((a, b) => (a.orderNum ?? 0) - (b.orderNum ?? 0) || a.id - b.id) .map((page) => page.id); if ( @@ -81,3 +83,27 @@ export function validateSiblingOrder( "A reorder must include every sibling exactly once", ); } + +/** + * Resolve the order number a newly created page should take under `parentId`. + * + * The emulator refuses to load a catalog whose siblings share an order number + * ("sibling order N is used more than once"), yet nearly every creation path + * passes a fixed 0. Honour an explicit request while it is still free, and + * otherwise append after the last sibling so creating a page can never + * introduce a duplicate in the first place. + */ +export function nextFreeSiblingOrder( + pages: readonly HierarchyPage[], + parentId: number, + requested?: number, +): number { + const orders = pages + .filter((page) => isSiblingOf(page, parentId)) + .map((page) => page.orderNum ?? 0); + const highest = orders.length ? Math.max(...orders) : 0; + if (requested === undefined || !Number.isFinite(requested)) + return highest + 1; + const wanted = Number(requested); + return orders.includes(wanted) ? highest + 1 : wanted; +} diff --git a/src/features/catalog/server/page-commands.ts b/src/features/catalog/server/page-commands.ts index 1b757915..c95258ed 100644 --- a/src/features/catalog/server/page-commands.ts +++ b/src/features/catalog/server/page-commands.ts @@ -9,6 +9,7 @@ import { CatalogNotFound, collectSubtree, type HierarchyPage, + nextFreeSiblingOrder, positiveId, validateSiblingOrder, } from "../domain/hierarchy"; @@ -102,9 +103,13 @@ export async function createPageCommand( if (!data.caption) throw new CatalogInputError("Category name is required"); return db.transaction(async (tx) => { const rows = await lockedPages(tx, kind); - assertParent(rows, 0, data.parentId ?? -1); + const parentId = data.parentId ?? -1; + assertParent(rows, 0, parentId); const assignments = fieldsSql(kind, { ...data, + // Never trust a caller-supplied order number: the emulator rejects a + // catalog where two siblings share one. + orderNum: nextFreeSiblingOrder(rows, parentId, data.orderNum), captionSave: data.captionSave ?? data.caption?.slice(0, 25), }); const [result] = await tx.execute(