From 213de59f945912602508e3163124bdc7640d25ee Mon Sep 17 00:00:00 2001 From: AI_Assistant Date: Sat, 5 Sep 2026 21:44:13 +1000 Subject: [PATCH 1/2] cmms-embed: fix cmms-config validation rejecting the real server response Browser-verified bug (2026-09-05, reported via the prod-side session): /api/cmms-config now returns 200 with aiEnabled:true, but the widget still never mounted. Root cause: isCmmsConfig() required entityType/entityId as strict strings, but the actual response omits them entirely - the whole payload failed validation and silently fell back to NO_WIDGET, with nothing logged to say why. Fix: only dark/aiEnabled/aiEditorKind actually gate anything - validate just those, and default csrfToken/entityType/entityId to "" when absent (they only feed the iframe's query string, harmless empty). Also added a .catch() + console.error on the widget's own boot effect, so a future mistake here logs instead of silently no-op'ing. Verified against the exact captured response body ({"dark":true,"aiEnabled":true,"aiEditorKind":"kicad","csrfToken":"..."}): now resolves aiEnabled:true. tsc --noEmit clean, vitest 469/469 passing. Also investigated the separately-reported "Uncaught Error: undefined self-hosted:8:1" console error: built the pre-patch source (0146488) and the patched source with an identical toolchain/base and diffed the eager main chunk - same file list, same wasm/collab/index.ts dynamic/static-import warning, ~4KB size delta matching only this patch's own new code. yjs/Y.Doc was ALREADY present in the pre-patch eager main chunk (grepped the unpatched build output directly), so this patch did not newly pull collab/yjs code into eager load - whatever's throwing predates this patch's chunk composition. Not provable further without a live browser (not available in this repo's env, see CLAUDE.md) - not a fix, just contrary evidence to "this patch's widget code caused it" for the prod-side session to browser-verify. Co-Authored-By: Claude Sonnet 5 --- .../src/components/CmmsAiWidget.tsx | 33 ++++++++++------- web/standalone/src/lib/cmms-config.ts | 36 +++++++++++++------ 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/web/standalone/src/components/CmmsAiWidget.tsx b/web/standalone/src/components/CmmsAiWidget.tsx index a4953f3..3aabf5f 100644 --- a/web/standalone/src/components/CmmsAiWidget.tsx +++ b/web/standalone/src/components/CmmsAiWidget.tsx @@ -20,18 +20,27 @@ export function CmmsAiWidget(): React.ReactElement | null { React.useEffect(() => { let cancelled = false; - fetchCmmsConfig().then((c) => { - if (cancelled) return; - setConfig(c); - if (c.aiEnabled) installAiBridge(); - // Belt-and-braces alongside the `?theme=` hand-off (lib/theme.ts) — that - // param already wins whenever edit.php sets it, so this only matters if - // it's ever absent. Skip if the URL param already ran; setTheme is - // otherwise idempotent (persists + toggles the same `.dark` class). - if (c.dark && !new URLSearchParams(window.location.search).get("theme")) { - setTheme("dark"); - } - }); + // fetchCmmsConfig() never rejects (its own internal .catch() falls back + // to NO_WIDGET) — this .catch() guards the callback body itself instead + // (setConfig/installAiBridge/setTheme), so a mistake here becomes a + // logged no-op, not a silent unhandled rejection with nothing to say why + // the widget didn't appear. + fetchCmmsConfig() + .then((c) => { + if (cancelled) return; + setConfig(c); + if (c.aiEnabled) installAiBridge(); + // Belt-and-braces alongside the `?theme=` hand-off (lib/theme.ts) — + // that param already wins whenever edit.php sets it, so this only + // matters if it's ever absent. Skip if the URL param already ran; + // setTheme is otherwise idempotent (persists + toggles `.dark`). + if (c.dark && !new URLSearchParams(window.location.search).get("theme")) { + setTheme("dark"); + } + }) + .catch((err: unknown) => { + console.error("[cmms-ai-widget] boot failed:", err); + }); return () => { cancelled = true; }; diff --git a/web/standalone/src/lib/cmms-config.ts b/web/standalone/src/lib/cmms-config.ts index 754e157..4ede25a 100644 --- a/web/standalone/src/lib/cmms-config.ts +++ b/web/standalone/src/lib/cmms-config.ts @@ -15,17 +15,22 @@ export interface CmmsConfig { entityId: string; } -function isCmmsConfig(v: unknown): v is CmmsConfig { +// Only `dark` / `aiEnabled` / `aiEditorKind` actually gate behavior — a +// bug found 2026-09-05 in browser verification required all six fields as +// strict strings, so the real server response (which omits `entityType` / +// `entityId`) failed validation and silently fell back to NO_WIDGET despite +// `aiEnabled: true` — the widget never mounted and nothing logged why. +// `csrfToken` / `entityType` / `entityId` are only used to build the iframe's +// query string (harmless as `""`), so a missing one shouldn't disable the +// widget entirely. +function isCmmsConfigCore(v: unknown): v is Pick { if (!v || typeof v !== "object") return false; const c = v as Record; - return ( - typeof c.dark === "boolean" && - typeof c.aiEnabled === "boolean" && - c.aiEditorKind === "kicad" && - typeof c.csrfToken === "string" && - typeof c.entityType === "string" && - typeof c.entityId === "string" - ); + return typeof c.dark === "boolean" && typeof c.aiEnabled === "boolean" && c.aiEditorKind === "kicad"; +} + +function optionalString(v: unknown): string { + return typeof v === "string" ? v : ""; } const NO_WIDGET: CmmsConfig = { @@ -47,7 +52,18 @@ export function fetchCmmsConfig(): Promise { if (!cached) { cached = fetch(`${API_BASE_URL}/api/cmms-config`, { credentials: "include" }) .then((r) => (r.ok ? r.json() : null)) - .then((v) => (isCmmsConfig(v) ? v : NO_WIDGET)) + .then((v): CmmsConfig => { + if (!isCmmsConfigCore(v)) return NO_WIDGET; + const c = v as Record; + return { + dark: v.dark, + aiEnabled: v.aiEnabled, + aiEditorKind: v.aiEditorKind, + csrfToken: optionalString(c.csrfToken), + entityType: optionalString(c.entityType), + entityId: optionalString(c.entityId), + }; + }) .catch(() => NO_WIDGET); } return cached; From dc2b1b5b19c248cdcdb00357084eb550ef03f673 Mon Sep 17 00:00:00 2001 From: AI_Assistant Date: Sun, 6 Sep 2026 14:51:35 +1000 Subject: [PATCH 2/2] =?UTF-8?q?cmms-embed:=20ai-bridge=20apply=20=E2=80=94?= =?UTF-8?q?=20carry=20newly-placed=20symbols'=20(lib=5Fsymbols)=20defs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cmms-ai-editor-apply handler diffed the proposed sheet with docDelta and pushed the item delta through applyDeltaToY. docDelta only diffs uuid-bearing items; a (lib_symbols ...) definition has no uuid, so an AI proposal that places a part the sheet has never carried landed the instance with no library definition — KiCad drew an instance pointing at an undefined symbol (nothing). applyToActiveBuffer now upserts the proposal's lib defs into kdoc_libsymbols (upsert by lib id, never deletes — the CMMS's kicad_splice_elements always splices into the existing block, so the proposal is existing ∪ additions) before the item delta, mirroring kicad-binding.ts's own DOWN path (upsertLibSymbolsToY(doc, defs, ORIGIN)). Deliberately not syncLayoutToY, which would also LWW-reconcile non-item layout heads (root uuid, sheet_instances, paper) from the proposal. New ai-bridge.test.ts covers it (fails without the upsert: the placed symbol's definition never reaches kdoc_libsymbols). Co-Authored-By: Claude Sonnet 5 --- .../src/wasm/collab/ai-bridge.test.ts | 104 ++++++++++++++++++ web/standalone/src/wasm/collab/ai-bridge.ts | 16 +++ 2 files changed, 120 insertions(+) create mode 100644 web/standalone/src/wasm/collab/ai-bridge.test.ts diff --git a/web/standalone/src/wasm/collab/ai-bridge.test.ts b/web/standalone/src/wasm/collab/ai-bridge.test.ts new file mode 100644 index 0000000..535a8da --- /dev/null +++ b/web/standalone/src/wasm/collab/ai-bridge.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from "vitest"; +import * as Y from "yjs"; +import { docToFile, docToY, kicadLibSymbolsMap, fileToDoc, yToDoc } from "@pcbjam/shared"; +import { applyToActiveBuffer, readActiveBuffer, registerActiveCollabDoc } from "./ai-bridge"; + +// A symbol instance is only renderable with its `(lib_symbols …)` definition. +// `docDelta` (the apply path's item differ) only diffs uuid-bearing items, and a +// `(symbol "Device:R" …)` lib def has no uuid — so an AI proposal that places a +// part the sheet has never carried used to land the instance with no definition, +// and KiCad drew nothing. `applyToActiveBuffer` now upserts the proposal's lib +// defs into `kdoc_libsymbols` alongside the item delta. + +const WIRE1 = "22222222-0000-0000-0000-000000000001"; +const SYM1 = "44444444-0000-0000-0000-000000000001"; + +const SCH_WITHOUT_SYMBOL = `(kicad_sch +\t(version 20250114) +\t(generator "eeschema") +\t(generator_version "9.0") +\t(uuid "11111111-1111-1111-1111-111111111111") +\t(paper "A4") +\t(lib_symbols) +\t(wire (pts (xy 50.8 50.8) (xy 101.6 50.8)) (stroke (width 0) (type default)) (uuid "${WIRE1}")) +\t(sheet_instances (path "/" (page "1"))) +) +`; + +const SCH_WITH_SYMBOL = `(kicad_sch +\t(version 20250114) +\t(generator "eeschema") +\t(generator_version "9.0") +\t(uuid "11111111-1111-1111-1111-111111111111") +\t(paper "A4") +\t(lib_symbols +\t\t(symbol "Device:R" (pin_numbers (hide yes)) (pin_names (offset 0)) (exclude_from_sim no) (in_bom yes) (on_board yes) +\t\t\t(property "Reference" "R" (at 2.032 0 90) (effects (font (size 1.27 1.27)))) +\t\t\t(property "Value" "R" (at 0 0 90) (effects (font (size 1.27 1.27)))) +\t\t\t(symbol "R_0_1" +\t\t\t\t(rectangle (start -1.016 -2.54) (end 1.016 2.54) (stroke (width 0.254) (type default)) (fill (type none))) +\t\t\t) +\t\t\t(symbol "R_1_1" +\t\t\t\t(pin passive line (at 0 3.81 270) (length 1.27) (name "~" (effects (font (size 1.27 1.27)))) (number "1" (effects (font (size 1.27 1.27))))) +\t\t\t\t(pin passive line (at 0 -3.81 90) (length 1.27) (name "~" (effects (font (size 1.27 1.27)))) (number "2" (effects (font (size 1.27 1.27))))) +\t\t\t) +\t\t) +\t) +\t(wire (pts (xy 50.8 50.8) (xy 101.6 50.8)) (stroke (width 0) (type default)) (uuid "${WIRE1}")) +\t(symbol (lib_id "Device:R") (at 63.5 63.5 0) (unit 1) (exclude_from_sim no) (in_bom yes) (on_board yes) (dnp no) +\t\t(uuid "${SYM1}") +\t\t(property "Reference" "R1" (at 66.04 62.23 0) (effects (font (size 1.27 1.27)) (justify left))) +\t\t(property "Value" "10k" (at 66.04 64.77 0) (effects (font (size 1.27 1.27)) (justify left))) +\t\t(pin "1" (uuid "44444444-0000-0000-0000-0000000000a1")) +\t\t(pin "2" (uuid "44444444-0000-0000-0000-0000000000a2")) +\t\t(instances (project "rt" (path "/11111111-1111-1111-1111-111111111111" (reference "R1") (unit 1)))) +\t) +\t(sheet_instances (path "/" (page "1"))) +) +`; + +function seed(src: string): Y.Doc { + const ydoc = new Y.Doc(); + docToY(fileToDoc(src), ydoc); + return ydoc; +} + +describe("ai-bridge applyToActiveBuffer", () => { + it("carries a newly-placed symbol's (lib_symbols) definition, not just the instance", () => { + const ydoc = seed(SCH_WITHOUT_SYMBOL); + registerActiveCollabDoc(ydoc, "board.kicad_sch"); + try { + expect(kicadLibSymbolsMap(ydoc).get("Device:R")).toBeUndefined(); + + applyToActiveBuffer(SCH_WITH_SYMBOL); + + const def = kicadLibSymbolsMap(ydoc).get("Device:R"); + expect(def).toBeDefined(); + expect(def).toContain("R_0_1"); + + // and the instance item itself landed via the normal item delta + const out = docToFile(yToDoc(ydoc)); + expect(out).toContain(SYM1); + expect(out).toContain('(lib_id "Device:R")'); + } finally { + registerActiveCollabDoc(null); + } + }); + + it("is a no-op on lib_symbols when the proposal adds no new definitions", () => { + const ydoc = seed(SCH_WITH_SYMBOL); + registerActiveCollabDoc(ydoc, "board.kicad_sch"); + try { + const before = kicadLibSymbolsMap(ydoc).get("Device:R"); + expect(before).toBeDefined(); + + // same doc, wire nudged — no new parts + applyToActiveBuffer(SCH_WITH_SYMBOL.replace("101.6 50.8", "127 50.8")); + + expect(kicadLibSymbolsMap(ydoc).get("Device:R")).toBe(before); + expect(readActiveBuffer()).toContain("127 50.8"); + } finally { + registerActiveCollabDoc(null); + } + }); +}); diff --git a/web/standalone/src/wasm/collab/ai-bridge.ts b/web/standalone/src/wasm/collab/ai-bridge.ts index 85a46cd..4040831 100644 --- a/web/standalone/src/wasm/collab/ai-bridge.ts +++ b/web/standalone/src/wasm/collab/ai-bridge.ts @@ -28,6 +28,8 @@ import { docDelta, docToFile, fileToDoc, + libSymbolsFromLayout, + upsertLibSymbolsToY, yToDoc, type KicadDoc, } from "@pcbjam/shared"; @@ -82,6 +84,20 @@ export function applyToActiveBuffer(newSource: string): void { const prev = yToDoc(activeDoc); const next = fileToDoc(newSource); const delta = docDelta(prev, next); + // `docDelta` only diffs uuid-bearing items — a `(lib_symbols …)` def has no + // uuid, so a proposal that places a symbol the sheet has never carried adds + // the instance item but drops its library definition, leaving KiCad an + // instance pointing at an undefined symbol (renders nothing). Push the + // proposal's lib defs into the kdoc_libsymbols map FIRST (upsert by lib id, + // never deletes — the server always splices into the existing block, so this + // is existing ∪ additions), so the items observer's `deltaToItemsWire` can + // prefix each new instance with its `(lib_symbols …)` on the way to the + // editor — exactly what `kicad-binding.ts`'s own DOWN path does with + // `upsertLibSymbolsToY(doc, defs, ORIGIN)`. Deliberately NOT `syncLayoutToY`: + // that also reconciles non-item layout heads (root `uuid`, `sheet_instances`, + // `paper`…) wholesale-LWW from the proposal, which would corrupt the sheet's + // identity if the server's spliced preamble deviates at all. + upsertLibSymbolsToY(activeDoc, libSymbolsFromLayout(next.layout, next.items), APPLY_ORIGIN); applyDeltaToY(activeDoc, delta, APPLY_ORIGIN); }