From ed42111df10f0f595e929cc377149ab996ede096 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sat, 31 Oct 2026 23:48:04 -0400 Subject: [PATCH] fix(filter-builder): OR-aware suggest constraints (user correction) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix was too blunt: under OR it sent NO constraint tree at all, so "library = ebooks" stopped narrowing the author menu the moment an OR-connected author rule appeared. The user spec is subtler: library = ebooks AND (author = Lewis Carroll OR author = ?) → the menu shows EBOOK authors, with Lewis Carroll OFF the list. suggestTree now walks root → edited rule and builds one conjunctive chain: - ALL-group siblings stay (AND narrows: the ebook scope persists); - ANY-group siblings drop — they are independent alternatives under OR and must not narrow; - EXCEPT a same-field ANY sibling, whose picked values NEGATE into the chain (contains → not_contains, is → is_not, in → not_in): the picked value leaves the menu, everything else in scope stays; - an ALREADY-NEGATED sibling picks nothing to take off, so it drops rather than flipping positive (a positive flip would narrow); - a field without a negation twin falls back to dropping; - incomplete rules never constrain (not filters yet), and the edited rule itself never constrains its own menu. Pins: the user's exact tree (ebook scope kept + Lewis Carroll negated off), the root-OR variant (different-field sibling drops, same-field negates), the twinless fallback, plus the existing AND/OR cases — 34 filter-builder vitest cases green, container filter slice green (TestFilterSuggestionsUnderRules unaffected: the server contract is unchanged, only which qf the client composes). --- web/src/filter_builder.test.ts | 62 ++++++++++++++++++- web/src/filter_builder.ts | 106 +++++++++++++++++++++++++-------- 2 files changed, 141 insertions(+), 27 deletions(-) diff --git a/web/src/filter_builder.test.ts b/web/src/filter_builder.test.ts index 220fa67f..f207a7a0 100644 --- a/web/src/filter_builder.test.ts +++ b/web/src/filter_builder.test.ts @@ -251,7 +251,10 @@ const builderFields = [ valueKind: "text" as const, defaultOp: "contains", suggest: true, - ops: [{ key: "contains", label: "contains", arity: "many" }], + ops: [ + { key: "contains", label: "contains", arity: "many" }, + { key: "not_contains", label: "does not contain", arity: "many" }, + ], }, ]; @@ -291,6 +294,63 @@ describe("mode-aware value suggestions", () => { expect(decodeURIComponent(captured.replace(/\+/g, " "))).toContain("Lewis Carroll"); }); + it("user spec: AND scope stays, the OR sibling's pick comes off the list", async () => { + const c = filterBuilderData(); + c.filterTree = { + mode: "all", + children: [ + { field: "library", op: "in", values: ["lib-ebooks"] }, + { + mode: "any" as const, + children: [ + { field: "author", op: "contains", values: ["Lewis Carroll"] }, + { field: "author", op: "contains", values: [""] }, + ], + }, + ], + }; + await c.fetchSuggestions("1-1", [1, 1], { field: "author", op: "contains", values: [""] }); + expect(captured).toContain("qf="); + const qf = decodeURIComponent(captured.split("qf=")[1]!.split("&")[0]!.replace(/\+/g, " ")); + // the ebook scope still narrows… + expect(qf).toContain('"field":"library"'); + expect(qf).toContain("lib-ebooks"); + // …and Lewis Carroll is REMOVED, not the only option + expect(qf).toContain("not_contains"); + expect(qf).toContain("Lewis Carroll"); + }); + + it("root OR: a different-field sibling drops, a same-field pick negates", async () => { + const c = filterBuilderData(); + c.filterTree = { + mode: "any", + children: [ + { field: "library", op: "in", values: ["lib-ebooks"] }, + { field: "author", op: "contains", values: ["Lewis Carroll"] }, + { field: "author", op: "contains", values: [""] }, + ], + }; + await c.fetchSuggestions("2", [2], { field: "author", op: "contains", values: [""] }); + expect(captured).toContain("qf="); + const qf = decodeURIComponent(captured.split("qf=")[1]!.split("&")[0]!.replace(/\+/g, " ")); + expect(qf).not.toContain("library"); + expect(qf).toContain("not_contains"); + expect(qf).toContain("Lewis Carroll"); + }); + + it("a same-field OR sibling without a negation twin just drops", async () => { + const c = filterBuilderData(); + c.filterTree = { + mode: "any", + children: [ + { field: "library", op: "in", values: ["lib-1"] }, + { field: "library", op: "in", values: [""] }, + ], + }; + await c.fetchSuggestions("1", [1], { field: "library", op: "in", values: [""] }); + expect(captured).not.toContain("qf="); + }); + it("sends NO sibling tree under OR — the menu browses the scope", async () => { const c = filterBuilderData(); c.filterTree = { diff --git a/web/src/filter_builder.ts b/web/src/filter_builder.ts index 8fe33da2..548945af 100644 --- a/web/src/filter_builder.ts +++ b/web/src/filter_builder.ts @@ -273,6 +273,80 @@ export function pruneIncomplete( return out; } +// completeRuleValues — the completeness test from pruneIncomplete: a +// value-taking rule is a filter only with trimmed, non-empty values +// (two-arity needs both). Null = not a filter yet. +function completeRuleValues(rule: FilterRuleState): string[] | null { + const arity = wireOps[rule.op]?.arity ?? "many"; + if (arity === "none") return []; + const vals = (rule.values ?? []).map((v) => (v ?? "").trim()); + const kept = vals.filter((v) => v !== ""); + if (kept.length === 0) return null; + if (arity === "two" && kept.length < 2) return null; + return arity === "two" ? vals : [kept.join(",")]; +} + +// negateRule — a same-field OR sibling's negated twin (contains ↔ +// not_contains, is ↔ is_not, in ↔ not_in): the sibling's picked values +// come OFF the menu while every other value stays ("we've selected +// Lewis Carroll — remove him, show the rest of the ebook authors"). +// An ALREADY-NEGATED sibling picks nothing, so it drops instead of +// flipping positive (a positive flip would narrow, not widen). Null = +// no twin (or nothing picked): the sibling just drops. +export function negateRule(rule: FilterRuleState): FilterRuleState | null { + if (rule.op.startsWith("not_")) return null; + const twin = opsForField(rule.field).find((o) => o.key === `not_${rule.op}`); + if (!twin) return null; + const vals = completeRuleValues(rule); + if (vals === null) return null; + return { field: rule.field, op: twin.key, values: vals }; +} + +// suggestTree — the constraint tree for one rule's value menu. The +// edited rule's own constraint is gone; walking root → edited rule: +// • ALL-group siblings stay (AND narrows candidates: "library = +// ebooks" keeps the menu to ebook authors); +// • ANY-group siblings are independent alternatives — they drop, +// EXCEPT a same-field sibling, whose picked values NEGATE into the +// chain (2026-10-31 user spec: OR must keep the ebook scope, take +// the already-picked author off the list, and offer the rest); +// • incomplete rules never constrain (they are not filters yet). +// The kept constraints are conjunctive, so the result is one ALL +// chain; null when nothing constrains (the caller sends no qf). +export function suggestTree( + group: FilterGroupState, + path: number[], + fieldKey: string, +): FilterGroupState | null { + if (path.length === 0) return null; + const onPath = group.children[path[0]]; + if (!onPath) return null; + const children: (FilterRuleState | FilterGroupState)[] = []; + group.children.forEach((child, idx) => { + if (idx === path[0]) return; + if (group.mode === "any") { + if (isRule(child) && child.field === fieldKey) { + const neg = negateRule(child); + if (neg) children.push(neg); + } + return; // OR siblings never narrow the menu + } + if (isRule(child)) { + const vals = completeRuleValues(child); + if (vals !== null) children.push({ field: child.field, op: child.op, values: vals }); + return; + } + const pruned = pruneIncomplete(child, null); + if (pruned && pruned.children.length > 0) children.push(pruned); + }); + if (path.length > 1 && !isRule(onPath)) { + const nested = suggestTree(onPath, path.slice(1), fieldKey); + if (nested && nested.children.length > 0) children.push(nested); + } + if (children.length === 0) return null; + return { mode: "all", children }; +} + // ─── row projection (flat rendering of the nested tree) ───────────── interface BuilderRow { @@ -634,22 +708,6 @@ export function filterBuilderData() { return FIELD_BY_KEY.get(node.field)?.valueKind === "id"; }, - // pathUnderAll — is every group from the root down to the edited - // rule's parent an AND ("all") group? Under any OR on the path the - // siblings cannot constrain the rule being edited. - pathUnderAll(path: number[]): boolean { - if (this.filterTree.mode !== "all") return false; - let node: FilterRuleState | FilterGroupState = this.filterTree; - for (let i = 0; i < path.length - 1; i++) { - if (isRule(node)) return false; - const child = (node as FilterGroupState).children[path[i]]; - if (!child || isRule(child)) return false; - node = child; - if (node.mode !== "all") return false; - } - return true; - }, - // valueCellText — what the value INPUT displays. Id-kind rules keep // the ids in rule.values (the clause matches ids); the cell shows // their LABELS, or the name text being typed. @@ -790,16 +848,12 @@ export function filterBuilderData() { this.suggestions[rowKey] = []; this.suggestionsLoading = rowKey; this.suggestionsOpen = rowKey; - // Symmetry: the tree sent EXCLUDES the rule being edited — the - // menu is computed under every OTHER COMPLETED rule (incomplete - // rules are not filters; sending them would be a 400). Under an - // OR connective on the edited rule's path (root mode "any" or an - // "any" ancestor group), siblings are INDEPENDENT alternatives — - // a new rule's candidates must not be narrowed by them — so no - // tree rides at all (2026-10-31 user report: with "Author - // contains Lewis Carroll" present, flipping to OR still starved - // the author menu down to Lewis Carroll). - const others = this.pathUnderAll(path) ? this.completedTree(path) : null; + // The menu is computed under the suggest-constraint tree: AND + // siblings keep narrowing, OR siblings drop (a same-field pick + // negates in, taking its value off the list), and the edited + // rule itself never constrains. Null = nothing constrains: no + // qf rides (an empty group is a server-side 400). + const others = suggestTree(this.filterTree, path, rule.field); const params = new URLSearchParams(); params.set(rule.field, typed || " "); if (others) params.set("qf", JSON.stringify(others));