From 19dc0347dfecca6d353b12b2ccb4795a6c57d5f9 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 11 Sep 2026 20:24:38 +0200 Subject: [PATCH] fix(studio): harden organize-imports against long-running queries and hangs The original GET route used a correlated NOT EXISTS / FIND_IN_SET subquery over the entire catalog_items table for every recent import audit entry, causing server timeouts when the audit log or catalog grew large. The per-item host-page validation inside the create action also issued one SELECT + one UPDATE per moved offer. Changes: - GET /api/admin/import/organize: replace the correlated subquery with a bounded candidate list and a JS-side placed-set check, then resolve all needed base items in a single indexed SELECT. This bounds the query cost regardless of catalog or audit log size. - organizeImportFurni action: validate mover ids in one SELECT, then batch every move per group into a single UPDATE with a CASE expression instead of one UPDATE per item. - OrganizeImportsDialog: add a 45-second abort timeout on the fetch and a distinct load-error state so the UI never silently hangs. - Add 'loadError' translation key (en + nl). --- src/actions/catalog.ts | 69 ++++++-- src/app/api/admin/import/organize/route.ts | 158 ++++++++++++------ .../admin/studio/organize-imports-dialog.tsx | 18 +- src/messages/en.json | 1 + src/messages/nl.json | 1 + 5 files changed, 181 insertions(+), 66 deletions(-) diff --git a/src/actions/catalog.ts b/src/actions/catalog.ts index 80b25037..c1be0f6f 100644 --- a/src/actions/catalog.ts +++ b/src/actions/catalog.ts @@ -451,9 +451,37 @@ export async function organizeImportFurni(input: { .where(inArray(ItemsBase.id, baseIds)); const baseMap = new Map(bases.map((b) => [b.id, b])); + // Verify every offer we're about to move still sits inside the import + // tree in ONE query, so we never hijack an unrelated catalog offer. + const moverIds = [ + ...new Set( + group.items + .map((r) => Number(r.catalogItemId)) + .filter((n) => Number.isFinite(n) && n > 0), + ), + ]; + const validMoveIds = new Set(); + if (moverIds.length > 0) { + const [hostRows] = (await db.execute(sql` + SELECT id, page_id FROM catalog_items WHERE id IN (${sql.join( + moverIds.map((id) => sql`${id}`), + sql`, `, + )}) + `)) as unknown as [Array<{ id: number; page_id: number }>, unknown]; + for (const row of hostRows) { + if (importPageIds.has(Number(row.page_id))) { + validMoveIds.add(Number(row.id)); + } + } + } + let nextOrder = 1; let moved = 0; let added = 0; + + const moveCaseOrder: string[] = []; + const moveCaseName: string[] = []; + const moveIds: number[] = []; for (const row of group.items) { const base = baseMap.get(row.itemId); if (!base) continue; @@ -461,23 +489,21 @@ export async function organizeImportFurni(input: { base.publicName || base.itemName || String(row.itemId); if (row.catalogItemId) { - // Verify the offer currently sits inside the import tree before - // moving it, so we never hijack an unrelated catalog offer. - const [host] = (await db.execute(sql` - SELECT page_id FROM catalog_items WHERE id = ${row.catalogItemId} - `)) as unknown as [Array<{ page_id: number }>, unknown]; - const hostPageId = Number(host[0]?.page_id ?? -1); - if (!importPageIds.has(hostPageId)) { - console.error( - `[organizeImportFurni] skipping offer #${row.catalogItemId} on non-import page #${hostPageId}`, - ); + if (!validMoveIds.has(Number(row.catalogItemId))) { + // Offer no longer lives in the import tree (moved elsewhere or + // deleted) — skip it instead of duplicating it. continue; } - await db.execute(sql` - UPDATE catalog_items - SET page_id = ${pageId}, order_number = ${nextOrder}, catalog_name = ${catalogName} - WHERE id = ${row.catalogItemId} - `); + moveCaseOrder.push( + `WHEN ${Number(row.catalogItemId)} THEN ${nextOrder}`, + ); + moveCaseName.push( + `WHEN ${Number(row.catalogItemId)} THEN '${catalogName.replace( + /['\\]/g, + "\\$&", + )}'`, + ); + moveIds.push(Number(row.catalogItemId)); moved++; } else { await insertCatalogItemRow({ @@ -502,6 +528,19 @@ export async function organizeImportFurni(input: { nextOrder++; } + if (moveIds.length > 0) { + await db.execute(sql` + UPDATE catalog_items + SET page_id = ${pageId}, + order_number = CASE id ${sql.raw(moveCaseOrder.join(" "))} END, + catalog_name = CASE id ${sql.raw(moveCaseName.join(" "))} END + WHERE id IN (${sql.join( + moveIds.map((id) => sql`${id}`), + sql`, `, + )}) + `); + } + await logStaffActivity({ staffId: staff.id, action: "catalog_page_create", diff --git a/src/app/api/admin/import/organize/route.ts b/src/app/api/admin/import/organize/route.ts index eb80ae04..f1329ec8 100644 --- a/src/app/api/admin/import/organize/route.ts +++ b/src/app/api/admin/import/organize/route.ts @@ -5,7 +5,7 @@ import { } from "@/features/catalog/server/import-pages"; import { apiOk } from "@/lib/api"; import { withAdmin } from "@/lib/api-handler"; -import { db } from "@/lib/db"; +import { db, ItemsBase } from "@/lib/db"; import { PERMS } from "@/lib/permissions"; interface OrganizeItem { @@ -21,14 +21,13 @@ interface OrganizeItem { alreadyPlaced: boolean; } -const columns = sql` - ib.id AS itemId, - ib.item_name AS itemName, - ib.public_name AS publicName, - ib.type AS type, - ib.sprite_id AS spriteId, - ib.interaction_type AS interactionType -`; +/** Parse the base-item ids stored in an offer's item_ids column. */ +function parseItemIds(raw: string): number[] { + return String(raw ?? "") + .split(/[;,]/) + .map((s) => Number(s.trim())) + .filter((n) => Number.isFinite(n) && n > 0); +} export const GET = withAdmin( { permission: PERMS.CATALOG_VIEW }, @@ -45,32 +44,38 @@ export const GET = withAdmin( const since = sinceIso.slice(0, 19).replace("T", " "); const pageIds = await getImportedCategoryPageIds(); - const byItemId = new Map(); - // 1) Offers already living in the imported-furniture tree. + // 1) Offers already living in the imported-furniture tree. No join, so + // this stays bounded by the (small) page-id set instead of scanning + // the whole catalog or forcing function-based lookups. + const placedById = new Map(); + const itemIdsToResolve = new Set(); if (pageIds.length > 0) { const [rows] = (await db.execute(sql` - SELECT ${columns}, - ci.id AS catalogItemId, + SELECT ci.id AS catalogItemId, ci.page_id AS sourcePageId, - p.caption AS sourcePageCaption + ci.item_ids AS itemIds, + p.caption AS sourcePageCaption, + ci.order_number AS orderNumber FROM catalog_items ci - INNER JOIN items_base ib ON ib.id = CAST(ci.item_ids AS UNSIGNED) INNER JOIN catalog_pages p ON p.id = ci.page_id WHERE ci.page_id IN (${sql.join(pageIds, sql`, `)}) ORDER BY ci.id LIMIT ${limit} `)) as unknown as [Record[], unknown]; + for (const r of rows) { - const itemId = Number(r.itemId); - if (!Number.isFinite(itemId)) continue; - byItemId.set(itemId, { - itemId, - itemName: String(r.itemName ?? ""), - publicName: String(r.publicName ?? ""), - type: String(r.type ?? "s"), - spriteId: Number(r.spriteId) || 0, - interactionType: String(r.interactionType ?? "default"), + const parsed = parseItemIds(String(r.itemIds ?? "")); + if (parsed.length === 0) continue; + const id = parsed[0]; + itemIdsToResolve.add(id); + placedById.set(id, { + itemId: id, + itemName: "", + publicName: String(r.sourcePageCaption ?? ""), + type: "s", + spriteId: 0, + interactionType: "default", catalogItemId: Number(r.catalogItemId), sourcePageId: Number(r.sourcePageId) || null, sourcePageCaption: r.sourcePageCaption @@ -81,45 +86,104 @@ export const GET = withAdmin( } } - // 2) Recently imported furniture that isn't in any catalog page yet. - const [auditRows] = (await db.execute(sql` - SELECT DISTINCT ${columns}, - NULL AS catalogItemId, - NULL AS sourcePageId, - NULL AS sourcePageCaption + // 2) Recently imported furniture. Bounded candidate list first, matched + // against the import tree in JS — never a correlated FIND_IN_SET scan. + const [candidateRows] = (await db.execute(sql` + SELECT DISTINCT alog.target_id AS itemId FROM admin_audit_log alog - INNER JOIN items_base ib ON ib.id = alog.target_id WHERE alog.action = 'furni_import' AND alog.target = 'ItemsBase' AND alog.target_id IS NOT NULL AND alog.created_at >= ${since} - AND NOT EXISTS ( - SELECT 1 FROM catalog_items ci - WHERE FIND_IN_SET(ib.id, REPLACE(ci.item_ids, ';', ',')) - ) ORDER BY itemId DESC - LIMIT ${limit} - `)) as unknown as [Record[], unknown]; - for (const r of auditRows) { + LIMIT 2000 + `)) as unknown as [Array<{ itemId: number }>, unknown]; + + for (const r of candidateRows) { const itemId = Number(r.itemId); if (!Number.isFinite(itemId)) continue; - if (byItemId.has(itemId)) continue; - byItemId.set(itemId, { + if (placedById.has(itemId)) continue; + if (itemIdsToResolve.size >= limit) break; + itemIdsToResolve.add(itemId); + } + + // 3) Resolve base-item metadata in ONE indexed query. + const baseMap = new Map< + number, + { + itemName: string; + publicName: string; + type: string; + spriteId: number; + interactionType: string; + } + >(); + if (itemIdsToResolve.size > 0) { + const bases = await db + .select({ + id: ItemsBase.id, + itemName: ItemsBase.itemName, + publicName: ItemsBase.publicName, + type: ItemsBase.type, + spriteId: ItemsBase.spriteId, + interactionType: ItemsBase.interactionType, + }) + .from(ItemsBase) + .where( + sql`${ItemsBase.id} IN (${sql.join( + [...itemIdsToResolve].map((id) => sql`${id}`), + sql`, `, + )})`, + ); + for (const b of bases) { + baseMap.set(b.id, { + itemName: String(b.itemName ?? ""), + publicName: String(b.publicName ?? ""), + type: String(b.type ?? "s"), + spriteId: Number(b.spriteId) || 0, + interactionType: String(b.interactionType ?? "default"), + }); + } + } + + // 4) Merge: placed offers (with metadata backfilled) first, then new. + const items: OrganizeItem[] = []; + for (const [id, entry] of placedById) { + const meta = baseMap.get(id); + items.push({ + ...entry, + itemName: meta?.itemName ?? entry.itemName, + publicName: meta?.publicName ?? entry.publicName, + type: meta?.type ?? entry.type, + spriteId: meta?.spriteId ?? entry.spriteId, + interactionType: meta?.interactionType ?? entry.interactionType, + }); + } + + let claimed = items.length; + for (const r of candidateRows) { + if (claimed >= limit) break; + const itemId = Number(r.itemId); + if (!Number.isFinite(itemId)) continue; + if (placedById.has(itemId)) continue; + const meta = baseMap.get(itemId); + if (!meta) continue; + items.push({ itemId, - itemName: String(r.itemName ?? ""), - publicName: String(r.publicName ?? ""), - type: String(r.type ?? "s"), - spriteId: Number(r.spriteId) || 0, - interactionType: String(r.interactionType ?? "default"), + itemName: meta.itemName, + publicName: meta.publicName, + type: meta.type, + spriteId: meta.spriteId, + interactionType: meta.interactionType, catalogItemId: null, sourcePageId: null, sourcePageCaption: null, alreadyPlaced: false, }); + claimed++; } - const items = [...byItemId.values()].sort((a, b) => { - // Already-placed furniture first, newest item id first within each set. + items.sort((a, b) => { return ( Number(b.alreadyPlaced) - Number(a.alreadyPlaced) || b.itemId - a.itemId ); diff --git a/src/components/admin/studio/organize-imports-dialog.tsx b/src/components/admin/studio/organize-imports-dialog.tsx index 19c72cee..0474552b 100644 --- a/src/components/admin/studio/organize-imports-dialog.tsx +++ b/src/components/admin/studio/organize-imports-dialog.tsx @@ -75,6 +75,7 @@ export function OrganizeImportsDialog({ const router = useRouter(); const [loading, setLoading] = useState(false); + const [loadError, setLoadError] = useState(false); const [items, setItems] = useState([]); const [rootPages, setRootPages] = useState([]); const [importRootPageId, setImportRootPageId] = useState(null); @@ -96,10 +97,17 @@ export function OrganizeImportsDialog({ const load = useCallback(async () => { setLoading(true); + setLoadError(false); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 45000); try { const [data, tree] = await Promise.all([ - fetch("/api/admin/import/organize").then((r) => r.json()), - fetch("/api/admin/catalog/tree?parentId=-1").then((r) => r.json()), + fetch("/api/admin/import/organize", { signal: controller.signal }).then( + (r) => r.json(), + ), + fetch("/api/admin/catalog/tree?parentId=-1", { + signal: controller.signal, + }).then((r) => r.json()), ]); setItems(Array.isArray(data?.items) ? data.items : []); setImportRootPageId(data?.importRootPageId ?? null); @@ -113,8 +121,10 @@ export function OrganizeImportsDialog({ ); } catch (error) { console.error("[OrganizeImports] load failed:", error); - toast.error("Failed to load imports"); + setLoadError(true); + setItems([]); } finally { + clearTimeout(timeout); setLoading(false); } }, []); @@ -347,7 +357,7 @@ export function OrganizeImportsDialog({ ) : items.length === 0 ? (
- {t("empty")} + {loadError ? t("loadError") : t("empty")}