From ee5c3bcbb3330db37bbbd44eb457d2b5ce980116 Mon Sep 17 00:00:00 2001 From: Dave Date: Fri, 16 Oct 2026 23:32:26 +0200 Subject: [PATCH] fix(quote-assistant): exact-SKU rows always shortlisted; per-line shortlist errors - With a SKU on the line, shortlistStockItems runs a second narrow query (sku = raw || sku ~ normalised) and merges by id, so the capped broad pull can't crowd out the exact SKU row. Quotes and backslashes are stripped from the raw SKU before any filter is built. - fillQuote catches a shortlist failure per line and marks only that line error instead of abandoning the quote. - Supplier names load once per fillQuote and are passed to each shortlist call (new optional parameter; default unchanged). Co-Authored-By: Claude Opus 5.5 --- .../server/quote-assistant-service.test.ts | 48 ++++++++++++++++ src/lib/server/quote-assistant-service.ts | 27 ++++++--- .../server/quote-assistant-shortlist.test.ts | 57 ++++++++++++++++++- src/lib/server/quote-assistant-shortlist.ts | 35 ++++++++++-- 4 files changed, 150 insertions(+), 17 deletions(-) diff --git a/src/lib/server/quote-assistant-service.test.ts b/src/lib/server/quote-assistant-service.test.ts index 8435c53..3630027 100644 --- a/src/lib/server/quote-assistant-service.test.ts +++ b/src/lib/server/quote-assistant-service.test.ts @@ -8,6 +8,7 @@ const store = { itemFilters: [] as string[], updates: [] as { id: string; patch: Record }[], failUpdateFor: new Set(), + supplierLoads: 0, }; const events: string[] = []; const actors: { name?: string; source?: string; ignoreRequestUser?: boolean }[] = []; @@ -59,6 +60,14 @@ const pb = { }, }; } + if (name === "suppliers") { + return { + getFullList: async () => { + store.supplierLoads += 1; + return [{ id: "sup1", suppliername: "Bolt Co" }]; + }, + }; + } throw new Error(`unexpected collection ${name}`); }, }; @@ -191,6 +200,7 @@ beforeEach(() => { store.itemFilters = []; store.updates = []; store.failUpdateFor.clear(); + store.supplierLoads = 0; events.length = 0; actors.length = 0; seq = 0; @@ -477,6 +487,44 @@ describe("fillQuote", () => { expect(forced.decide.mock.calls[0][0].lines.map((l) => l.quote_item_id)).toEqual(["a"]); }); + it("marks only the line whose shortlist failed as error; the rest still run", async () => { + addQuote("q1"); + const a = addItem("q1", { id: "a", description: "breaks the shortlist" }); + const b = addItem("q1", { id: "b", description: "fine" }); + const { deps, decide, shortlist } = makeDeps(); + shortlist.mockImplementation(async (_pb: unknown, line: { description: string }) => { + if (line.description === "breaks the shortlist") throw new Error("PB filter error"); + return [candidate()]; + }); + + const r = await fillQuote("q1", { deps }); + + expect(r.filled).toBe(1); + expect(r.error).toContain("PB filter error"); + expect(decide.mock.calls[0][0].lines.map((l) => l.quote_item_id)).toEqual([b]); + const failed = store.items.get(a)!; + expect((failed.ai_fill as { status: string }).status).toBe("error"); + expect(failed.pricing_notes).toBe(""); + expect((store.items.get(b)!.ai_fill as { status: string }).status).toBe("filled"); + }); + + it("loads the supplier names once per quote and hands them to every shortlist call", async () => { + addQuote("q1"); + addItem("q1"); + addItem("q1"); + addItem("q1"); + const { deps, shortlist } = makeDeps(); + + await fillQuote("q1", { deps }); + + expect(store.supplierLoads).toBe(1); + expect(shortlist).toHaveBeenCalledTimes(3); + const maps = shortlist.mock.calls.map((c) => (c as unknown[])[3]); + expect(maps[0]).toBeInstanceOf(Map); + expect((maps[0] as Map).get("sup1")).toBe("Bolt Co"); + expect(maps.every((m) => m === maps[0])).toBe(true); + }); + it("does not throw when a single write fails; reports it and continues", async () => { addQuote("q1"); const a = addItem("q1", { id: "a" }); diff --git a/src/lib/server/quote-assistant-service.ts b/src/lib/server/quote-assistant-service.ts index 38dbeaf..b498b6b 100644 --- a/src/lib/server/quote-assistant-service.ts +++ b/src/lib/server/quote-assistant-service.ts @@ -38,6 +38,7 @@ import { type StockCandidate, } from "@/lib/server/quote-assistant-shortlist"; import { escapeForPbString } from "@/lib/server/price-list-match"; +import { loadSuppliersById } from "@/lib/server/item-intel-service"; import { isAiFillPending } from "@/lib/quote-assistant-review"; import type { Quote, QuoteAssistantRunSummary, QuoteItem } from "@/lib/types"; @@ -122,18 +123,21 @@ export async function fillQuote( .slice(0, maxLines); if (lines.length === 0) return result; - // 1. Shortlist POS candidates per line (deterministic, no AI). + // 1. Shortlist POS candidates per line (deterministic, no AI). One + // line's lookup failing marks only that line as an error. + const supplierNameById = await loadSuppliersById(pb); const candidatesById = new Map(); - try { - for (const item of lines) { + const shortlistFailed = new Set(); + for (const item of lines) { + try { candidatesById.set( item.id, - await shortlist(pb, { description: item.description, sku: item.sku }), + await shortlist(pb, { description: item.description, sku: item.sku }, undefined, supplierNameById), ); + } catch (e) { + shortlistFailed.add(item.id); + if (!result.error) result.error = `shortlist failed for line ${item.id}: ${errMessage(e)}`; } - } catch (e) { - result.error = `shortlist failed: ${errMessage(e)}`; - return result; } // 2. One AI call for the lines that have candidates. @@ -186,8 +190,13 @@ export async function fillQuote( minConfidence: settings.quote_assistant_min_confidence, model, now, - status: - candidates.length === 0 ? "no_candidates" : aiFailed || noAnswer ? "error" : undefined, + status: shortlistFailed.has(line.id) + ? "error" + : candidates.length === 0 + ? "no_candidates" + : aiFailed || noAnswer + ? "error" + : undefined, }); await pb.collection("quote_items").update(line.id, patch); const status = (patch.ai_fill as { status?: string } | undefined)?.status; diff --git a/src/lib/server/quote-assistant-shortlist.test.ts b/src/lib/server/quote-assistant-shortlist.test.ts index 9ad5256..e46587f 100644 --- a/src/lib/server/quote-assistant-shortlist.test.ts +++ b/src/lib/server/quote-assistant-shortlist.test.ts @@ -88,10 +88,61 @@ describe("shortlistStockItems", () => { expect(out.map((r) => r.id)).toContain("s1"); }); - it("includes the escaped SKU in the PB filter", async () => { + it("strips quotes and backslashes from the raw SKU before building filters", async () => { const { pb, getList } = makePb([]); - await shortlistStockItems(pb, { description: "", sku: 'AB"123' }); - expect(getList.mock.calls[0][2].filter).toContain('sku ~ "AB\\"123"'); + await shortlistStockItems(pb, { description: "", sku: 'AB"12\\3' }); + for (const call of getList.mock.calls) { + const filter = call[2].filter!; + expect(filter).not.toContain("\\"); + expect(filter).not.toMatch(/AB"|AB\\"/); + } + expect(getList.mock.calls[0][2].filter).toContain('sku ~ "AB123"'); + }); + + it("runs a narrow SKU query and ranks its exact hit first even when the broad pull missed it", async () => { + // The broad token pull is capped (500 rows, unsorted): simulate it + // filling up with lookalikes that don't include the SKU row. + const decoys: Row[] = Array.from({ length: 500 }, (_, i) => ({ + id: `d${i}`, sku: `X-${i}`, name: "hex bolt zinc", description: "hex bolt zinc", + })); + const skuRow: Row = { id: "exact", sku: "hb-10/50", name: "Bolt", description: "something else", cost_price: 4 }; + const { pb, getList } = makePb([]); + getList.mockImplementation(async (_p, _n, opts) => + ({ items: opts.filter!.includes('sku = "') ? [skuRow] : decoys }), + ); + + const out = await shortlistStockItems(pb, { description: "hex bolt zinc", sku: "HB 10 50" }); + + expect(getList).toHaveBeenCalledTimes(2); + const narrow = getList.mock.calls[1][2].filter!; + expect(narrow).toBe('is_active = true && (sku = "HB 10 50" || sku ~ "HB1050")'); + expect(out[0].id).toBe("exact"); + expect(out[0].score).toBe(1); + }); + + it("merges the narrow and broad results by id (no duplicates)", async () => { + const skuRow: Row = { id: "sku1", sku: "NUT-M18", name: "M18 NUT 8 PLN", description: "M18 NUT 8 PLN" }; + const { pb } = makePb([skuRow, NUT]); + const out = await shortlistStockItems(pb, { description: "M18 hex nut grade 8", sku: "NUT-M18" }); + expect(out.map((r) => r.id).filter((id) => id === "sku1")).toHaveLength(1); + }); + + it("makes no narrow query without a SKU", async () => { + const { pb, getList } = makePb([NUT]); + await shortlistStockItems(pb, { description: "M18 hex nut grade 8" }); + expect(getList).toHaveBeenCalledTimes(1); + }); + + it("uses a supplier map passed in instead of loading suppliers", async () => { + const { pb, getFullList } = makePb([NUT]); + const [row] = await shortlistStockItems( + pb, + { description: "M18 hex nut grade 8" }, + 8, + new Map([["sup1", "Preloaded Supplier"]]), + ); + expect(row.supplier_name).toBe("Preloaded Supplier"); + expect(getFullList).not.toHaveBeenCalled(); }); it("drops rows scoring below 0.2", async () => { diff --git a/src/lib/server/quote-assistant-shortlist.ts b/src/lib/server/quote-assistant-shortlist.ts index a2de8fb..3b4d057 100644 --- a/src/lib/server/quote-assistant-shortlist.ts +++ b/src/lib/server/quote-assistant-shortlist.ts @@ -7,6 +7,10 @@ * the top informative tokens, then in-app Jaccard/overlap scoring. Differences: * only active items, name + description + sku are searched, and an exact * (normalised) SKU hit always wins with score 1. + * + * The broad pull is capped and unsorted, so with a SKU on the line a second + * narrow query (exact / normalised SKU) makes sure the SKU row is never + * crowded out; both result sets are merged by id before scoring. */ import type { getAdminPocketBase } from "@/lib/server/pocketbase-admin"; import { @@ -35,6 +39,8 @@ export interface StockCandidate { export type AdminPb = Awaited>; const FETCH_LIMIT = 500; +const SKU_FETCH_LIMIT = 50; +const STOCK_FIELDS = "id,sku,name,description,notes,unit,cost_price,selling_price,markup,supplier"; const MIN_SCORE = 0.2; const MAX_TOKENS = 4; @@ -51,13 +57,20 @@ type StockRow = { supplier?: string; }; +/** + * `supplierNameById`: pass a preloaded supplier map when shortlisting many + * lines in a row; when omitted the suppliers are loaded here. + */ export async function shortlistStockItems( pb: AdminPb, line: { description: string; sku?: string }, limit = 8, + supplierNameById?: Map, ): Promise { const tokens = informativeTokens(line.description || "", MAX_TOKENS); - const rawSku = (line.sku || "").trim(); + // `"` and `\` can't be escaped reliably in a PB filter string and never + // matter to a SKU match (normaliseSku drops them anyway). + const rawSku = (line.sku || "").replace(/[\\"]/g, "").trim(); const wantedSku = normaliseSku(rawSku); const clauses: string[] = []; @@ -70,11 +83,23 @@ export async function shortlistStockItems( const res = await pb.collection("stock_items").getList(1, FETCH_LIMIT, { filter: `is_active = true && (${clauses.join(" || ")})`, - fields: "id,sku,name,description,notes,unit,cost_price,selling_price,markup,supplier", + fields: STOCK_FIELDS, }); + const rowsById = new Map(); + if (rawSku) { + const skuClauses = [`sku = "${escapeForPbString(rawSku)}"`]; + if (wantedSku) skuClauses.push(`sku ~ "${escapeForPbString(wantedSku)}"`); + const narrow = await pb.collection("stock_items").getList(1, SKU_FETCH_LIMIT, { + filter: `is_active = true && (${skuClauses.join(" || ")})`, + fields: STOCK_FIELDS, + }); + for (const row of narrow.items) rowsById.set(row.id, row); + } + for (const row of res.items) if (!rowsById.has(row.id)) rowsById.set(row.id, row); + const lineTokens = tokenizeIntel(line.description || ""); - const scored = res.items + const scored = [...rowsById.values()] .map((row) => { const skuHit = wantedSku !== "" && normaliseSku(row.sku) === wantedSku; const score = skuHit @@ -91,7 +116,7 @@ export async function shortlistStockItems( if (scored.length === 0) return []; - const supplierNameById = await loadSuppliersById(pb); + const suppliers = supplierNameById ?? (await loadSuppliersById(pb)); return scored.map(({ row, score }) => ({ id: row.id, name: row.name || "", @@ -103,7 +128,7 @@ export async function shortlistStockItems( selling_price: row.selling_price || 0, markup: row.markup || 0, supplier_id: row.supplier || "", - supplier_name: supplierNameById.get(row.supplier || "") || "", + supplier_name: suppliers.get(row.supplier || "") || "", score: +score.toFixed(3), })); }