fix(mcp-server): make edit_diagram all-or-nothing and fix preview sync races

- edit_diagram applies nothing when any operation fails, rejects invalid or multi-cell new_xml, validates only the target page, and returns the current page XML on every rejection (including stale edits) - Fix get_diagram reading the old diagram right after an AI write: the preview pushed its sync reply with a newer version than it was taken at - Keep a user edit that loses the race with an AI write in history and tell the user in the preview - Autofix removes only exact foreign tags (a stray <mxGraph/> deleted <mxGraphModel>), fixes tag case, drops orphan <mxPoint>s, and rejects unknown element names in model XML - Edit empty and compressed pages; PNG exports use the page on screen; tag download exports; reload from the server after a page export - Expand ~ in paths, tell the model when the browser sync timed out, use registerPrompt, require SDK ^1.31.0
2026-10-11 20:19:51 +08:00 · 2026-10-03 22:03:44 +09:00
parent a46787c1b8
commit 6d67a0ec69
13 changed files with 712 additions and 204 deletions
@@ -4,6 +4,7 @@
 * The id sits on the wrapper; the inner mxCell has none.
 */

+import { deflateRawSync } from "node:zlib"
 import { beforeAll, describe, expect, it, vi } from "vitest"
 import { installDomPolyfill } from "../src/dom.js"

@@ -83,3 +84,43 @@ describe("cascade delete logging", () => {
        spy.mockRestore()
    })
 })
+
+describe("pages without a <root>", () => {
+    const ADD_A = {
+        operation: "add" as const,
+        cell_id: "a",
+        new_xml: `<mxCell id="a" vertex="1" parent="1"><mxGeometry as="geometry"/></mxCell>`,
+    }
+
+    it("treats an empty page as a blank page", () => {
+        const doc = `<mxfile><diagram id="p" name="Page-1"></diagram></mxfile>`
+        const { result, errors } = applyDiagramOperations(doc, [ADD_A])
+        expect(errors).toEqual([])
+        expect(result).toContain('<mxCell id="0"/>')
+        expect(result).toContain('<mxCell id="1" parent="0"/>')
+        expect(result).toContain('<mxCell id="a"')
+    })
+
+    it("decompresses a compressed page before editing it", () => {
+        const model = `<mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/><mxCell id="b" vertex="1" parent="1"/></root></mxGraphModel>`
+        const compressed = deflateRawSync(
+            Buffer.from(encodeURIComponent(model)),
+        ).toString("base64")
+        const doc = `<mxfile><diagram id="p" name="Page-1">${compressed}</diagram></mxfile>`
+        const { result, errors } = applyDiagramOperations(doc, [ADD_A])
+        expect(errors).toEqual([])
+        expect(result).not.toContain(compressed)
+        expect(result).toContain('<mxCell id="b"')
+        expect(result).toContain('<mxCell id="a"')
+        // Only one model and one set of root cells
+        expect(result.match(/<mxGraphModel/g)).toHaveLength(1)
+        expect(result.match(/<mxCell id="0"/g)).toHaveLength(1)
+    })
+
+    it("reports a page whose text is not compressed XML", () => {
+        const doc = `<mxfile><diagram id="p" name="Page-1">not base64 !!</diagram></mxfile>`
+        const { errors } = applyDiagramOperations(doc, [ADD_A])
+        expect(errors[0]?.cellId).toBe("")
+        expect(errors[0]?.message).toContain("could not be decompressed")
+    })
+})
@@ -0,0 +1,130 @@
+/**
+ * Tests for the all-or-nothing edit_diagram core (src/edit-diagram.ts).
+ */
+
+import { beforeAll, describe, expect, it } from "vitest"
+import { installDomPolyfill } from "../src/dom.js"
+
+beforeAll(() => {
+    installDomPolyfill()
+})
+
+import { editDiagram, targetPageXml } from "../src/edit-diagram.js"
+import { validateMxCellStructure } from "../src/xml-validation.js"
+
+const cell = (id: string, extra = "") =>
+    `<mxCell id="${id}" value="${id}" vertex="1" parent="1"${extra}><mxGeometry x="0" y="0" width="80" height="40" as="geometry"/></mxCell>`
+
+const page = (id: string, cells: string) =>
+    `<diagram id="${id}" name="${id}"><mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/>${cells}</root></mxGraphModel></diagram>`
+
+const DOC = `<mxfile>${page("p1", cell("a") + cell("b"))}</mxfile>`
+
+describe("editDiagram", () => {
+    it("applies every operation and counts them", () => {
+        const out = editDiagram(
+            DOC,
+            [
+                { operation: "add", cell_id: "c", new_xml: cell("c") },
+                { operation: "delete", cell_id: "b" },
+            ],
+            {},
+        )
+        expect(out.ok).toBe(true)
+        if (!out.ok) return
+        expect(out.applied).toBe(2)
+        expect(out.xml).toContain('id="c"')
+        expect(out.xml).not.toContain('id="b"')
+    })
+
+    it("applies nothing when one operation fails", () => {
+        const out = editDiagram(
+            DOC,
+            [
+                { operation: "add", cell_id: "c", new_xml: cell("c") },
+                { operation: "delete", cell_id: "missing" },
+            ],
+            {},
+        )
+        expect(out.ok).toBe(false)
+        if (out.ok) return
+        expect(out.pageError).toBe(false)
+        expect(out.errors).toEqual([
+            'delete missing: Cell with id="missing" not found',
+        ])
+    })
+
+    it("rejects new_xml that is still invalid after auto-fix", () => {
+        const out = editDiagram(
+            DOC,
+            [
+                {
+                    operation: "update",
+                    cell_id: "a",
+                    new_xml: `<mxCell id="a" style="x" style="y" vertex="1" parent="1"/>`,
+                },
+            ],
+            {},
+        )
+        expect(out.ok).toBe(false)
+        if (out.ok) return
+        expect(out.errors[0]).toMatch(/^update a: invalid new_xml: /)
+    })
+
+    it("rejects several cells in one new_xml", () => {
+        const out = editDiagram(
+            DOC,
+            [
+                {
+                    operation: "add",
+                    cell_id: "c",
+                    new_xml: cell("c") + cell("d"),
+                },
+            ],
+            {},
+        )
+        expect(out.ok).toBe(false)
+        if (out.ok) return
+        expect(out.errors[0]).toContain("exactly one cell")
+    })
+
+    it("accepts a UserObject that wraps one mxCell", () => {
+        const wrapped = `<UserObject id="u" label="U" link="https://example.com"><mxCell vertex="1" parent="1"><mxGeometry as="geometry"/></mxCell></UserObject>`
+        const out = editDiagram(
+            DOC,
+            [{ operation: "add", cell_id: "u", new_xml: wrapped }],
+            {},
+        )
+        expect(out.ok).toBe(true)
+    })
+
+    it("is not blocked by a problem on another page", () => {
+        // Page p2 has a duplicate cell id, which fails validation
+        const doc = `<mxfile>${page("p1", cell("a"))}${page("p2", cell("x") + cell("x"))}</mxfile>`
+        expect(validateMxCellStructure(doc)).not.toBeNull()
+        const out = editDiagram(
+            doc,
+            [{ operation: "add", cell_id: "c", new_xml: cell("c") }],
+            { page_id: "p1" },
+        )
+        expect(out.ok).toBe(true)
+    })
+
+    it("reports a missing page as a page-level error", () => {
+        const out = editDiagram(DOC, [{ operation: "delete", cell_id: "a" }], {
+            page_id: "nope",
+        })
+        expect(out.ok).toBe(false)
+        if (out.ok) return
+        expect(out.pageError).toBe(true)
+    })
+})
+
+describe("targetPageXml", () => {
+    it("returns only the selected page", () => {
+        const doc = `<mxfile>${page("p1", cell("a"))}${page("p2", cell("z"))}</mxfile>`
+        const xml = targetPageXml(doc, { page_id: "p2" })
+        expect(xml).toContain('id="z"')
+        expect(xml).not.toContain('id="a"')
+    })
+})
@@ -8,12 +8,14 @@

 import http from "node:http"
 import { afterAll, beforeAll, describe, expect, it } from "vitest"
-import { addHistory } from "../src/history.js"
+import { addHistory, getHistory } from "../src/history.js"
 import {
    getState,
+    requestSync,
    setState,
    shutdown,
    startHttpServer,
+    waitForSync,
 } from "../src/http-server.js"

 let port = 0
@@ -189,6 +191,55 @@ describe("POST /api/state", () => {
            expect(getState(id)?.xml).toBe(xml)
        }
    })
+
+    it("keeps a rejected user edit in history", async () => {
+        const id = "mcp-conflict-history"
+        setState(id, "<mxfile>user v1</mxfile>", undefined, true)
+        const aiVersion = setState(id, "<mxfile>AI edit</mxfile>")
+        const before = getHistory(id).length
+
+        const stale = await postJson("/api/state", {
+            sessionId: id,
+            xml: "<mxfile>lost user edit</mxfile>",
+            baseVersion: aiVersion - 1,
+        })
+        expect(stale.status).toBe(409)
+        expect(JSON.parse(stale.body).savedToHistory).toBe(true)
+        const history = getHistory(id)
+        expect(history).toHaveLength(before + 1)
+        expect(history.at(-1)?.xml).toBe("<mxfile>lost user edit</mxfile>")
+    })
+
+    it("ends a pending sync when the sync reply is older than an AI write", async () => {
+        const id = "mcp-stale-sync"
+        setState(id, "<mxfile>before</mxfile>", undefined, true)
+        const aiVersion = setState(id, "<mxfile>AI edit</mxfile>")
+        requestSync(id)
+        const before = getHistory(id).length
+
+        // The browser exported its old diagram, then loaded the AI write
+        const stale = await postJson("/api/state", {
+            sessionId: id,
+            xml: "<mxfile>before</mxfile>",
+            baseVersion: aiVersion - 1,
+            source: "sync",
+        })
+        expect(stale.status).toBe(409)
+        expect(JSON.parse(stale.body).savedToHistory).toBe(false)
+        expect(getState(id)?.xml).toBe("<mxfile>AI edit</mxfile>")
+        expect(getState(id)?.syncRequested).toBeUndefined()
+        expect(getHistory(id)).toHaveLength(before)
+        expect(await waitForSync(id, 200)).toBe(true)
+    })
+})
+
+describe("preview page", () => {
+    it("serves a script that parses", async () => {
+        const res = await request("/?mcp=mcp-test-script")
+        const script = res.body.match(/<script>([\s\S]*)<\/script>/)?.[1]
+        expect(script).toBeTruthy()
+        expect(() => new Function(script as string)).not.toThrow()
+    })
 })

 describe("history restore", () => {
@@ -107,7 +107,7 @@ afterAll(() => {
 })

 describe("MCP server wiring", () => {
-    it("registers all nine multi-page tools", async () => {
+    it("registers all ten tools", async () => {
        const resp = await send("tools/list", {})
        expect(resp.error, JSON.stringify(resp.error)).toBeUndefined()
        const names: string[] = (resp.result?.tools ?? []).map(
@@ -184,3 +184,65 @@ describe("XML serializer and strict parsing in page helpers", () => {
        ).toThrow()
    })
 })
+
+describe("autoFixXml keeps valid tags", () => {
+    it("removes a stray <mxGraph/> without touching <mxGraphModel>", () => {
+        const r = validateAndFixXml(
+            model(`<mxGraph/><mxCell id="2" vertex="1" parent="1"/>`),
+        )
+        expect(r.valid).toBe(true)
+        expect(r.fixed).toContain("<mxGraphModel>")
+        expect(r.fixed).toContain("</mxGraphModel>")
+        expect(r.fixed).not.toContain("<mxGraph/>")
+        expect(r.fixed).toContain('id="2"')
+    })
+
+    it("removes a stray <a> without touching <Array> waypoints", () => {
+        const edge = `<mxCell id="e" edge="1" parent="1"><mxGeometry relative="1" as="geometry"><Array as="points"><mxPoint x="1" y="2"/></Array></mxGeometry></mxCell>`
+        const r = validateAndFixXml(model(`<a>x</a>${edge}`))
+        expect(r.valid).toBe(true)
+        expect(r.fixed).toContain('<Array as="points">')
+        expect(r.fixed).toContain('<mxPoint x="1" y="2"/>')
+        expect(r.fixed).not.toContain("<a>")
+    })
+
+    it("fixes a lowercase <mxcell> instead of deleting every cell", () => {
+        const r = validateAndFixXml(
+            model(
+                `<mxcell id="3" vertex="1" parent="1"></mxcell>${BROKEN_CELL}`,
+            ),
+        )
+        expect(r.valid).toBe(true)
+        expect(r.fixed).toContain('<mxCell id="3"')
+        expect(r.fixed).toContain('<mxCell id="0"/>')
+        expect(r.fixed).toContain('value="R&amp;D"')
+    })
+
+    it("keeps label text when removing a foreign tag", () => {
+        const cell = `<mxCell id="4" value="&lt;b&gt;Bold&lt;/b&gt;" vertex="1" parent="1"/>`
+        const r = validateAndFixXml(model(`<foo/>${cell}`))
+        expect(r.valid).toBe(true)
+        expect(r.fixed).toContain('value="&lt;b&gt;Bold&lt;/b&gt;"')
+        expect(r.fixed).not.toContain("<foo/>")
+    })
+})
+
+describe("validateAndFixXml strict checks", () => {
+    it("fixes the case of an unknown element name", () => {
+        const r = validateAndFixXml(
+            model(`<mxcell id="3" vertex="1" parent="1"/>`),
+        )
+        expect(r.valid).toBe(true)
+        expect(r.fixes).toContain("Fixed tag case of <mxCell>")
+        expect(r.fixed).toContain('<mxCell id="3"')
+    })
+
+    it("removes an orphan mxPoint but keeps waypoints and named points", () => {
+        const cell = `<mxCell id="e" edge="1" parent="1"><mxGeometry relative="1" as="geometry"><mxPoint x="5" y="5"/><mxPoint x="0" y="0" as="sourcePoint"/><Array as="points"><mxPoint x="1" y="2"/></Array></mxGeometry></mxCell>`
+        const r = validateAndFixXml(model(cell))
+        expect(r.valid).toBe(true)
+        expect(r.fixed).not.toContain('<mxPoint x="5" y="5"/>')
+        expect(r.fixed).toContain('as="sourcePoint"')
+        expect(r.fixed).toContain('<mxPoint x="1" y="2"/>')
+    })
+})