mirror of
https://github.com/DayuanJiang/next-ai-draw-io.git
synced 2026-10-12 04:29:51 +08:00
fix(mcp-server): count a one-page view only for that page, and more review fixes
Found by the second PR review: - get_diagram with a page selector, or a rejected edit's error, counted the whole document as seen, so an edit on another page could overwrite the user's change there. A one-page view now counts for all pages only if the others are unchanged; otherwise the reply says to get them. - add_page accepted shapes with the root cell ids "0" and "1" and renamed them, breaking their edges. The check also missed ids on UserObject wrappers and ids written with spaces around the "=". - Root cells written over two lines were kept as an extra layer, cells with id = "a" did not count as cells, and CDATA text before a page's model passed the check although draw.io cannot open the page. - Auto-save cleanup deleted the user's own files that start with mcp-. Only names in the session id format are removed now. - Restoring a history entry dropped edits made in the browser since the last entry. They are added to history first. - A session whose state expired showed a blank page, and the next change overwrote its auto-save file. The saved file is loaded instead. - An edit on a page export's one-page projection, made before the real document was back, replaced the whole document. - A late sync reply could overwrite a newer edit: each sync export is numbered, and the server ignores replies older than the current state. - screenshot_diagram could return another session's image after start_session ran during its retries.
This commit is contained in:
@@ -16,7 +16,11 @@ beforeAll(() => {
|
||||
installDomPolyfill()
|
||||
})
|
||||
|
||||
import { checkEditGate, contentFingerprint } from "../src/edit-gate.ts"
|
||||
import {
|
||||
checkEditGate,
|
||||
contentFingerprint,
|
||||
markPageSeen,
|
||||
} from "../src/edit-gate.ts"
|
||||
|
||||
const XML_A = `<mxfile host="app.diagrams.net"><diagram id="p1" name="Page-1"><mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/><mxCell id="box1" value="Hello" style="rounded=0;" vertex="1" parent="1"><mxGeometry x="40" y="40" width="120" height="60" as="geometry"/></mxCell></root></mxGraphModel></diagram></mxfile>`
|
||||
|
||||
@@ -130,3 +134,31 @@ describe("contentFingerprint", () => {
|
||||
expect(contentFingerprint("not xml at all")).toBe("not xml at all")
|
||||
})
|
||||
})
|
||||
|
||||
describe("markPageSeen", () => {
|
||||
const page = (id: string, label: string) =>
|
||||
`<diagram id="${id}" name="${id}"><mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/><mxCell id="c" value="${label}" vertex="1" parent="1"/></root></mxGraphModel></diagram>`
|
||||
const doc = (a: string, b: string) =>
|
||||
`<mxfile>${page("A", a)}${page("B", b)}</mxfile>`
|
||||
|
||||
it("counts the whole document as seen when the other pages are unchanged", () => {
|
||||
const seen = doc("a1", "b1")
|
||||
const live = doc("a2", "b1")
|
||||
expect(markPageSeen(seen, live, { page_id: "A" })).toBe(live)
|
||||
})
|
||||
|
||||
it("does not count a changed page the model was not shown", () => {
|
||||
// The user edited page B; the model looked at page A only
|
||||
const seen = doc("a1", "b1")
|
||||
const live = doc("a1", "b2")
|
||||
const marked = markPageSeen(seen, live, { page_id: "A" })
|
||||
expect(marked).toBe(seen)
|
||||
expect(checkEditGate(marked, live).ok).toBe(false)
|
||||
})
|
||||
|
||||
it("counts everything as seen when the model saw nothing before", () => {
|
||||
// It has no old copy of the other pages to edit from
|
||||
const live = doc("a1", "b1")
|
||||
expect(markPageSeen("", live, { page_id: "A" })).toBe(live)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -11,6 +11,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"
|
||||
import { addHistory, getHistory } from "../src/history.ts"
|
||||
import {
|
||||
getState,
|
||||
onSessionRecreate,
|
||||
requestExport,
|
||||
requestSync,
|
||||
setState,
|
||||
@@ -242,6 +243,27 @@ describe("POST /api/state", () => {
|
||||
expect(getHistory(id)).toHaveLength(before)
|
||||
expect(await waitForSync(id, 200)).toBe(true)
|
||||
})
|
||||
|
||||
it("ignores a sync reply older than a user edit saved meanwhile", async () => {
|
||||
const id = "mcp-late-sync"
|
||||
const version = setState(id, "<mxfile>A</mxfile>", undefined, true)
|
||||
requestSync(id)
|
||||
// The user's edit is saved before the sync reply arrives
|
||||
const edit = await postJson("/api/state", {
|
||||
sessionId: id,
|
||||
xml: "<mxfile>B</mxfile>",
|
||||
baseVersion: version,
|
||||
})
|
||||
expect(edit.status).toBe(200)
|
||||
const late = await postJson("/api/state", {
|
||||
sessionId: id,
|
||||
xml: "<mxfile>A</mxfile>",
|
||||
baseVersion: version,
|
||||
source: "sync",
|
||||
})
|
||||
expect(late.status).toBe(409)
|
||||
expect(getState(id)?.xml).toBe("<mxfile>B</mxfile>")
|
||||
})
|
||||
})
|
||||
|
||||
describe("export requests", () => {
|
||||
@@ -283,6 +305,19 @@ describe("export requests", () => {
|
||||
})
|
||||
|
||||
describe("preview page", () => {
|
||||
it("shows the saved diagram of a session whose state expired", async () => {
|
||||
const saved = `<mxfile><diagram id="p" name="P"><mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/><mxCell id="kept" vertex="1" parent="1"/></root></mxGraphModel></diagram></mxfile>`
|
||||
onSessionRecreate((id) => (id === "mcp-expired" ? saved : null))
|
||||
try {
|
||||
await request("/?mcp=mcp-expired")
|
||||
expect(getState("mcp-expired")?.xml).toBe(saved)
|
||||
await request("/?mcp=mcp-never-saved")
|
||||
expect(getState("mcp-never-saved")?.xml).not.toContain("kept")
|
||||
} finally {
|
||||
onSessionRecreate(() => null)
|
||||
}
|
||||
})
|
||||
|
||||
it("serves scripts that parse, with every placeholder filled", async () => {
|
||||
const res = await request("/?mcp=mcp-test-script")
|
||||
expect(res.body).not.toContain("{{")
|
||||
@@ -314,4 +349,24 @@ describe("history restore", () => {
|
||||
expect(res.status).toBe(200)
|
||||
expect(getState(id)?.xml).toBe("<mxfile>5</mxfile>")
|
||||
})
|
||||
|
||||
it("keeps manual edits in history before restoring", async () => {
|
||||
const id = "mcp-history-manual"
|
||||
const doc = (cellId: string) =>
|
||||
`<mxfile><diagram id="p" name="P"><mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/><mxCell id="${cellId}" vertex="1" parent="1"/></root></mxGraphModel></diagram></mxfile>`
|
||||
const version = setState(id, doc("ai"))
|
||||
addHistory(id, doc("ai"))
|
||||
// An edit in the browser is not a history entry by itself
|
||||
const push = await postJson("/api/state", {
|
||||
sessionId: id,
|
||||
xml: doc("manual"),
|
||||
baseVersion: version,
|
||||
})
|
||||
expect(push.status).toBe(200)
|
||||
|
||||
const [entry] = getHistory(id)
|
||||
await postJson("/api/restore", { sessionId: id, id: entry.id })
|
||||
expect(getState(id)?.xml).toBe(doc("ai"))
|
||||
expect(getHistory(id).map((e) => e.xml)).toContain(doc("manual"))
|
||||
})
|
||||
})
|
||||
|
||||
@@ -52,19 +52,38 @@ describe("Autosaver", () => {
|
||||
it("keeps only the newest session files and never touches other files", () => {
|
||||
const dir = tempDir()
|
||||
const saver = new Autosaver(dir, 10, 2)
|
||||
for (const [i, id] of ["mine", "mcp-old", "mcp-mid"].entries()) {
|
||||
// Session ids look like mcp-<time in base 36>-<random>
|
||||
const ids = [
|
||||
"mine",
|
||||
"mcp-notes",
|
||||
"mcp-system-design-v2",
|
||||
"mcp-mgd0a1b2-old123",
|
||||
"mcp-mgd0a1b3-mid456",
|
||||
]
|
||||
for (const [i, id] of ids.entries()) {
|
||||
writeFileSync(join(dir, `${id}.drawio`), DIAGRAM)
|
||||
utimesSync(join(dir, `${id}.drawio`), 1000 + i, 1000 + i)
|
||||
}
|
||||
saver.schedule("mcp-new", DIAGRAM)
|
||||
saver.schedule("mcp-mgd0a1b4-new789", DIAGRAM)
|
||||
saver.flush()
|
||||
expect(readdirSync(dir).sort()).toEqual([
|
||||
"mcp-mid.drawio",
|
||||
"mcp-new.drawio",
|
||||
"mcp-mgd0a1b3-mid456.drawio",
|
||||
"mcp-mgd0a1b4-new789.drawio",
|
||||
"mcp-notes.drawio",
|
||||
"mcp-system-design-v2.drawio",
|
||||
"mine.drawio",
|
||||
])
|
||||
})
|
||||
|
||||
it("reads a session's saved diagram back", () => {
|
||||
const saver = new Autosaver(tempDir(), 10)
|
||||
expect(saver.load("mcp-none")).toBeNull()
|
||||
saver.schedule("mcp-back", DIAGRAM)
|
||||
saver.flush()
|
||||
expect(saver.load("mcp-back")).toBe(DIAGRAM)
|
||||
expect(new Autosaver(null).load("mcp-back")).toBeNull()
|
||||
})
|
||||
|
||||
it("does nothing when saving is off", () => {
|
||||
const saver = new Autosaver(null)
|
||||
expect(saver.pathFor("mcp-x")).toBeNull()
|
||||
|
||||
@@ -10,7 +10,7 @@ beforeAll(() => {
|
||||
installDomPolyfill()
|
||||
})
|
||||
|
||||
import { prepareNewDiagram } from "../src/new-diagram.ts"
|
||||
import { prepareNewDiagram, reservedIdError } from "../src/new-diagram.ts"
|
||||
import { hasCells, wrapCellsInModel } from "../src/pages.ts"
|
||||
import { validateAndFixXml } from "../src/xml-validation.ts"
|
||||
|
||||
@@ -59,6 +59,15 @@ describe("wrapCellsInModel", () => {
|
||||
expect(wrapCellsInModel(file)).toBe(file)
|
||||
})
|
||||
|
||||
it("replaces root cells written over two lines", () => {
|
||||
const wrapped = wrapCellsInModel(
|
||||
`<mxCell id="0">\n</mxCell>\n<mxCell id="1" parent="0">\n</mxCell>\n${A}`,
|
||||
)
|
||||
expect(wrapped).toBe(
|
||||
`<mxGraphModel><root>${ROOTS}${A}</root></mxGraphModel>`,
|
||||
)
|
||||
})
|
||||
|
||||
it("keeps a UserObject cell at the end", () => {
|
||||
const wrapped = `<UserObject id="u" label="U"><mxCell vertex="1" parent="1"><mxGeometry as="geometry"/></mxCell></UserObject>`
|
||||
expect(wrapCellsInModel(A + wrapped)).toContain(wrapped)
|
||||
@@ -75,6 +84,35 @@ describe("hasCells", () => {
|
||||
false,
|
||||
)
|
||||
})
|
||||
|
||||
it("counts cells with spaces around the =", () => {
|
||||
expect(hasCells(`<mxCell id = "a" vertex="1" parent="1"/>`)).toBe(true)
|
||||
expect(
|
||||
hasCells(`<mxCell id = "0"/><mxCell id = "1" parent="0"/>`),
|
||||
).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe("reservedIdError", () => {
|
||||
it("finds a shape that uses a root cell id", () => {
|
||||
expect(reservedIdError(A)).toBeNull()
|
||||
expect(reservedIdError(ROOTS + A)).toBeNull()
|
||||
expect(
|
||||
reservedIdError(`<mxCell id = "1" vertex="1" parent="1"/>`),
|
||||
).toMatch(/"0" and "1"/)
|
||||
// A linked or labelled shape has its id on the wrapper
|
||||
expect(
|
||||
reservedIdError(
|
||||
`<UserObject id="1" label="Docs"><mxCell vertex="1" parent="1"><mxGeometry as="geometry"/></mxCell></UserObject>`,
|
||||
),
|
||||
).toMatch(/"0" and "1"/)
|
||||
// A whole model has its own root cells
|
||||
expect(
|
||||
reservedIdError(
|
||||
`<mxGraphModel><root>${ROOTS}</root></mxGraphModel>`,
|
||||
),
|
||||
).toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
describe("prepareNewDiagram", () => {
|
||||
|
||||
@@ -317,4 +317,12 @@ describe("text directly under a page", () => {
|
||||
expect(r.valid).toBe(true)
|
||||
expect(r.fixed).not.toContain("\\n")
|
||||
})
|
||||
|
||||
it("rejects CDATA text before the model of a page", () => {
|
||||
const r = validateAndFixXml(
|
||||
`<mxfile><diagram id="p" name="P"><![CDATA[not-base64!]]>${model(`<mxCell id="2" vertex="1" parent="1"/>`)}</diagram></mxfile>`,
|
||||
)
|
||||
expect(r.valid).toBe(false)
|
||||
expect(r.error).toMatch(/not-base64/)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user