fix(mcp-server): review fixes for get_selection

Server (src/http-server.ts, src/index.ts, src/selection.ts,
src/new-diagram.ts):
- overlapping get_selection calls take turns (readSelection, one slot per
  session as the export slot) instead of replacing each other's request,
  which left one of them with a false "tab not in front" timeout
- an answer is taken only while a request is pending (both ids undefined
  compared equal)
- the tool sees the page start_session actually opens: with dist/shell
  missing the classic page is in use, which never answers, so the tool
  says so instead of timing out
- the result lists at most 100 cells and counts the rest: a whole large
  diagram selected would fill the model's context
- every <diagram> the model sends without an id gets one, so the page id
  the shell reports exists in the server's document

Shell (shell/mcp-sync-core.ts, shell/use-mcp-sync.ts,
contexts/diagram-context.tsx, lib/drawio/editor-bridge.ts):
- an answer whose POST failed is sent again at the next poll
- no answer while a full load has yet to reach the editor: it still shows
  the previous document, whose cells and pages the answer would name
- a hidden tab (the same session open twice) answers a poll later, so the
  tab in front answers first; alone, it still answers within the timeout
- a cell's container is reported by the model's isLayer, not by comparing
  with the default parent, which is the group the user entered
This commit is contained in:
dayuan.jiang
2026-10-11 20:56:07 +09:00
parent 734a31d4eb
commit 467fd25fd1
12 changed files with 316 additions and 31 deletions
+4
View File
@@ -115,6 +115,9 @@ interface DiagramContextType {
* another page): not recorded as the diagram, its autosaves ignored,
* until the next loadDiagram puts the real document back */
showTransient: (xml: string) => void
/** A full load was sent and draw.io has not reported it yet: the
* editor still shows the previous document */
hasPendingLoad: () => boolean
isDrawioReady: boolean
onDrawioLoad: () => void
resetDrawioReady: () => void
@@ -597,6 +600,7 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) {
captureValidationPng,
requestExport,
showTransient,
hasPendingLoad: () => pendingLoadsRef.current > 0,
isDrawioReady,
onDrawioLoad,
resetDrawioReady,
+4 -2
View File
@@ -610,7 +610,6 @@ export function readSelectionDetails(): SelectionAnswer | null {
const g = graph()
if (!g) return null
const page = ui?.currentPage
const defaultParent = g.getDefaultParent?.()
const cells = (g.getSelectionCells() as any[])
.filter((cell) => cell?.id)
.map((cell) => {
@@ -635,8 +634,11 @@ export function readSelectionDetails(): SelectionAnswer | null {
}
}
}
// The container the cell is in; a layer is none (and the
// default parent is the group the user entered, if any, so it
// cannot tell the two apart)
const parent = g.model.getParent(cell)
if (parent?.id && parent !== defaultParent) {
if (parent?.id && !g.model.isLayer(parent)) {
info.parent = String(parent.id)
}
return info
+36 -9
View File
@@ -97,6 +97,9 @@ export interface SyncCanvas {
currentXml(): string
/** The page on screen; null when unknown (external draw.io) */
currentPageId(): string | null
/** A full load was sent and draw.io has not reported it yet: the
* editor still shows the previous document */
loadPending(): boolean
/** The cells selected in the editor, for get_selection; `unavailable`
* when the page cannot reach the editor (external draw.io) */
readSelection(): SelectionAnswer
@@ -122,6 +125,8 @@ export interface SyncOptions {
canvas: SyncCanvas
onNotice: (notice: SyncNotice) => void
onStatus?: (status: SyncStatus) => void
/** The tab is hidden; another tab of this session may be in front */
isHidden?: () => boolean
/** The server made a new state for the session: History entries got
* new ids, so a list on screen is stale */
onStateRecreated?: () => void
@@ -736,20 +741,42 @@ export function createMcpSync(options: SyncOptions): McpSync {
}
// A selection request (get_selection): the editor's selection
// on the page the user is viewing, once draw.io is up (the
// bridge attaches to it at its first load), and not while a
// projection replaces that page
// bridge attaches to it at its first load), not while a
// projection replaces that page, and not while a full load (the
// one above, or one still on its way) has yet to reach the
// editor: it still shows the previous document
if (
s.selectionId &&
s.selectionId !== answeredSelectionId &&
isReady &&
!projectionExportActive
!projectionExportActive &&
!canvas.loadPending()
) {
answeredSelectionId = s.selectionId
postJson("/state", {
sessionId,
selectionId: s.selectionId,
selection: canvas.readSelection(),
}).catch(() => {})
const id = s.selectionId
answeredSelectionId = id
const answer = () =>
postJson("/state", {
sessionId,
selectionId: id,
selection: canvas.readSelection(),
})
.then((r) => {
if (!r.ok) throw new Error(String(r.status))
})
.catch(() => {
// Not delivered: the next poll answers it again
if (answeredSelectionId === id) {
answeredSelectionId = null
}
})
// A hidden tab lets a tab in front (the same session open
// twice) answer first; the server takes the first answer
// and ignores the later one
if (options.isHidden?.()) {
setTimeout(answer, POLL_INTERVAL_MS + 500)
} else {
answer()
}
}
} catch {
setStatus("offline")
@@ -56,6 +56,7 @@ export function useMcpSync(config: ShellConfig): {
diagramRef.current.requestExport(request, timeoutMs),
currentXml: () => diagramRef.current.chartXMLRef.current,
currentPageId: () => useCanvasStore.getState().currentPageId,
loadPending: () => diagramRef.current.hasPendingLoad(),
// The bridge reads the editor directly; without it (an
// external draw.io) the selection cannot be read
readSelection: () =>
@@ -69,6 +70,7 @@ export function useMcpSync(config: ShellConfig): {
onNotice: (notice) =>
toast(dictRef.current.shell[notice], { duration: 8000 }),
onStatus: setStatus,
isHidden: () => document.visibilityState === "hidden",
})
syncRef.current = sync
setSync(sync)
+31 -5
View File
@@ -126,14 +126,21 @@ export function previewUiFromEnv(
export const PREVIEW_UI: PreviewUi = previewUiFromEnv()
/**
* The page start_session actually opens: the classic page stands in while
* the shell is not built (a run from the sources), as embed.diagrams.net
* does for a missing dist/drawio
*/
export function activePreviewUi(ui: PreviewUi = PREVIEW_UI): PreviewUi {
return ui === "shell" && shellDir === null ? "classic" : ui
}
export function previewUrl(
port: number,
sessionId: string,
ui: PreviewUi = PREVIEW_UI,
): string {
// The classic page stands in while the shell is not built (a run from
// the sources), as embed.diagrams.net does for a missing dist/drawio
const path = ui === "shell" && shellDir !== null ? "/shell/" : ""
const path = activePreviewUi(ui) === "shell" ? "/shell/" : ""
return `http://localhost:${port}${path}?mcp=${sessionId}`
}
@@ -445,6 +452,24 @@ export async function waitForSelection(
return answer ?? null
}
// One selection slot per session, as the export slot: overlapping
// get_selection calls take turns instead of replacing each other's request
let selectionQueue: Promise<unknown> = Promise.resolve()
/** requestSelection + waitForSelection, one call at a time; null when the
* session is unknown or the tab does not answer in time */
export function readSelection(
sessionId: string,
timeoutMs = 10000,
): Promise<SelectionAnswer | null> {
const run = selectionQueue.then(() => {
if (!requestSelection(sessionId)) return null
return waitForSelection(sessionId, timeoutMs)
})
selectionQueue = run.catch(() => {})
return run
}
/**
* What the version cards depend on: History's entries (their count and the
* newest id) and the entry the canvas shows. The shell reads History again
@@ -466,7 +491,8 @@ function handleSelectionResult(
data: { selectionId?: unknown; selection?: unknown },
): void {
const state = stateStore.get(sessionId)
if (!state || data.selectionId !== state.selectionId) {
// Only the pending request's answer (none pending: none is taken)
if (!state?.selectionId || data.selectionId !== state.selectionId) {
log.debug(`Ignored a late selection answer for session=${sessionId}`)
return
}
@@ -515,7 +541,7 @@ export function startHttpServer(port = 6002): Promise<number> {
"No bundled draw.io (dist/drawio missing); the preview loads it from embed.diagrams.net",
)
}
if (PREVIEW_UI === "shell" && shellDir === null) {
if (PREVIEW_UI === "shell" && activePreviewUi() === "classic") {
log.warn(
"The canvas shell is not built (dist/shell missing); start_session opens the classic page",
)
+7 -8
View File
@@ -48,6 +48,7 @@ import {
otherVersions,
} from "./history.ts"
import {
activePreviewUi,
DRAWIO_BASE_URL,
type ExportFormat,
type ExportOptions,
@@ -57,17 +58,15 @@ import {
keepInHistory,
onSessionRecreate,
onStateChange,
PREVIEW_UI,
previewUrl,
readSelection,
requestExport,
requestSelection,
requestSync,
restoreHistoryEntry,
restoreSavedSession,
setState,
shutdown,
startHttpServer,
waitForSelection,
waitForSync,
} from "./http-server.ts"
import { parseDrawioFileContent } from "./load-diagram.ts"
@@ -1262,13 +1261,14 @@ server.registerTool(
}
const sessionId = currentSession.id
// The classic preview page never answers: it has no access to
// the editor (the shell reads it through the same-origin frame)
if (PREVIEW_UI !== "shell") {
// the editor (the shell reads it through the same-origin frame).
// It is also what opens while the shell is not built
if (activePreviewUi() !== "shell") {
return {
content: [
{
type: "text",
text: "The classic preview page (DRAWIO_PREVIEW_UI=classic) cannot read the selection. Ask the user which shapes they mean, or call get_diagram.",
text: "The classic preview page cannot read the selection (DRAWIO_PREVIEW_UI=classic, or the canvas shell is not built: run npm run build in packages/mcp-server). Ask the user which shapes they mean, or call get_diagram.",
},
],
}
@@ -1287,8 +1287,7 @@ server.registerTool(
isError: true,
}
}
requestSelection(sessionId)
const answer = await waitForSelection(sessionId)
const answer = await readSelection(sessionId)
if (!answer) {
return {
content: [
+23 -2
View File
@@ -3,7 +3,14 @@
* and the web app's display_diagram tool.
*/
import { expandCompactCells } from "./compact-cells.ts"
import { hasCells, normalizeToMxfile, wrapCellsInModel } from "./pages.ts"
import {
generatePageId,
hasCells,
normalizeToMxfile,
parseMxfile,
serializeMxfile,
wrapCellsInModel,
} from "./pages.ts"
import {
addDefaultStyles,
applyStyleClasses,
@@ -182,12 +189,26 @@ export function prepareNewDiagram(
error: `XML validation failed after expanding the cells - ${rewriteError}`,
}
}
const normalized = normalizeToMxfile(xml, page)
let normalized = normalizeToMxfile(xml, page)
if (!normalized) {
return {
ok: false,
error: "XML must be the mxCell elements of one page, a <mxGraphModel>, or an <mxfile> with one or more <diagram> children.",
}
}
// A <diagram> the model sent without an id gets one here. draw.io would
// otherwise make its own, which the server never sees, and get_selection
// would then name a page edit_diagram cannot find
const doc = parseMxfile(normalized)
if (doc) {
let added = false
doc.querySelectorAll("diagram").forEach((d) => {
if (!d.getAttribute("id")) {
d.setAttribute("id", generatePageId())
added = true
}
})
if (added) normalized = serializeMxfile(doc)
}
return { ok: true, xml: normalized, fixes }
}
+13 -2
View File
@@ -69,10 +69,14 @@ export function parseSelectionAnswer(value: unknown): SelectionAnswer | null {
return { pageId: text(given.pageId), pageName: text(given.pageName), cells }
}
/** describeSelection lists at most this many cells; the rest are counted */
export const MAX_LISTED_CELLS = 100
/**
* The get_selection result text. `externalDrawio` names the draw.io origin
* when the preview loads it from another origin, where the page cannot
* read the editor.
* read the editor. A selection of a whole large diagram is counted, not
* listed in full: the model needs the ids of a few hundred cells at most.
*/
export function describeSelection(
answer: SelectionAnswer,
@@ -89,7 +93,8 @@ export function describeSelection(
if (answer.cells.length === 0) {
return `Nothing is selected on ${page}. Ask the user to select the shapes they mean in the preview, or call get_diagram.`
}
const lines = answer.cells.map((cell) => {
const listed = answer.cells.slice(0, MAX_LISTED_CELLS)
const lines = listed.map((cell) => {
const label = cell.label ? `"${cell.label}"` : "(no label)"
if (cell.edge) {
const ends =
@@ -105,6 +110,12 @@ export function describeSelection(
const parent = cell.parent ? ` inside "${cell.parent}"` : ""
return `- id="${cell.id}" shape ${label}${where}${parent}`
})
const more = answer.cells.length - listed.length
if (more > 0) {
lines.push(
`- and ${more} more cells not listed here; call get_diagram for the rest`,
)
}
const count =
answer.cells.length === 1 ? "1 cell" : `${answer.cells.length} cells`
const target = answer.pageId ? ` (page_id="${answer.pageId}")` : ""
+73 -1
View File
@@ -20,6 +20,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"
import { installDomPolyfill } from "../src/dom.ts"
import { addHistory, getHistory } from "../src/history.ts"
import {
activePreviewUi,
drawioEmbedParams,
getApiToken,
getState,
@@ -29,6 +30,7 @@ import {
onStateChange,
previewUiFromEnv,
previewUrl,
readSelection,
requestExport,
requestSelection,
requestSync,
@@ -488,6 +490,72 @@ describe("selection requests (get_selection)", () => {
expect(getState(id)?.selectionId).toBeUndefined()
expect(requestSelection("mcp-unknown-session")).toBe(false)
})
it("takes no answer while no request is pending", async () => {
const id = "mcp-selection-unasked"
setState(id, "<mxfile>x</mxfile>")
// Both undefined would compare equal
await postJson("/api/state", { sessionId: id, selection: picked })
expect(getState(id)?.selection).toBeUndefined()
requestSelection(id)
const { selectionId } = getState(id) ?? {}
await postJson("/api/state", { sessionId: id, selection: picked })
expect(getState(id)?.selection).toBeUndefined()
expect(getState(id)?.selectionId).toBe(selectionId)
await waitForSelection(id, 50)
})
it("keeps the first answer when two tabs answer the same request", async () => {
const id = "mcp-selection-two-tabs"
setState(id, "<mxfile>x</mxfile>")
requestSelection(id)
const { selectionId } = getState(id) ?? {}
await postJson("/api/state", {
sessionId: id,
selectionId,
selection: picked,
})
const other = { ...picked, cells: [] }
await postJson("/api/state", {
sessionId: id,
selectionId,
selection: other,
})
expect(await waitForSelection(id, 1000)).toEqual(picked)
})
it("lets overlapping get_selection calls take turns, each getting its own answer", async () => {
const id = "mcp-selection-overlap"
setState(id, "<mxfile>x</mxfile>")
const poll = async () =>
JSON.parse((await request(`/api/state?sessionId=${id}`)).body)
const first = readSelection(id, 3000)
const second = readSelection(id, 3000)
// The tab sees one request and answers it
let shown = await poll()
expect(shown.selectionId).toMatch(/^[0-9a-f-]{36}$/)
await postJson("/api/state", {
sessionId: id,
selectionId: shown.selectionId,
selection: picked,
})
expect(await first).toEqual(picked)
// Then the second call's request, another id, answered in turn
await new Promise((r) => setTimeout(r, 150))
const next = await poll()
expect(next.selectionId).toMatch(/^[0-9a-f-]{36}$/)
expect(next.selectionId).not.toBe(shown.selectionId)
const other = { ...picked, cells: [] }
await postJson("/api/state", {
sessionId: id,
selectionId: next.selectionId,
selection: other,
})
expect(await second).toEqual(other)
shown = await poll()
expect(shown.selectionId).toBeNull()
expect(await readSelection("mcp-unknown-session", 100)).toBeNull()
})
})
describe("a session state recreated after it was lost", () => {
@@ -944,12 +1012,16 @@ describe("canvas shell page", () => {
expect(previewUrl(6002, "mcp-x", "classic")).toBe(
"http://localhost:6002?mcp=mcp-x",
)
// Not built: the classic page, which works, rather than a 404
// Not built: the classic page, which works, rather than a 404; the
// tools that need the shell (get_selection) see the classic page too
expect(activePreviewUi("shell")).toBe("shell")
expect(activePreviewUi("classic")).toBe("classic")
setShellDir(null)
try {
expect(previewUrl(6002, "mcp-x", "shell")).toBe(
"http://localhost:6002?mcp=mcp-x",
)
expect(activePreviewUi("shell")).toBe("classic")
} finally {
setShellDir(shellDir)
}
@@ -1,6 +1,7 @@
import { describe, expect, it } from "vitest"
import {
describeSelection,
MAX_LISTED_CELLS,
parseSelectionAnswer,
type SelectionAnswer,
} from "../src/selection.ts"
@@ -52,6 +53,29 @@ describe("get_selection text", () => {
expect(text).toContain('edit_diagram (page_id="p1")')
})
it("counts a large selection instead of listing every cell", () => {
const cells = (n: number) =>
Array.from({ length: n }, (_, i) => ({
id: `c${i + 1}`,
label: `Cell ${i + 1}`,
edge: false,
geometry: { x: i, y: 0, width: 10, height: 10 },
}))
const text = describeSelection(
{ ...page, cells: cells(MAX_LISTED_CELLS + 5) },
null,
)
expect(text).toContain(`${MAX_LISTED_CELLS + 5} cells selected`)
expect(text).toContain(`id="c${MAX_LISTED_CELLS}"`)
expect(text).not.toContain(`id="c${MAX_LISTED_CELLS + 1}"`)
expect(text).toContain("and 5 more cells not listed here")
const exact = describeSelection(
{ ...page, cells: cells(MAX_LISTED_CELLS) },
null,
)
expect(exact).not.toContain("more cells")
})
it("says clearly when nothing is selected", () => {
const text = describeSelection({ ...page, cells: [] }, null)
expect(text).toBe(
@@ -146,6 +146,24 @@ describe("reservedIdError", () => {
})
describe("prepareNewDiagram", () => {
it("gives every <diagram> without an id one, so the server and draw.io name the same pages", () => {
// draw.io makes up an id for a page without one, which the server
// never sees; get_selection would then name a page edit_diagram
// cannot find
const out = prepareNewDiagram(
`<mxfile><diagram name="One"><mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/></root></mxGraphModel></diagram><diagram name="Two" id="kept"><mxGraphModel><root><mxCell id="0"/><mxCell id="1" parent="0"/></root></mxGraphModel></diagram></mxfile>`,
)
expect(out.ok).toBe(true)
if (!out.ok) return
const ids = [...out.xml.matchAll(/<diagram\b[^>]*\bid="([^"]*)"/g)].map(
(m) => m[1],
)
expect(ids).toHaveLength(2)
expect(ids[0]).toMatch(/^[a-z0-9]+-[a-z0-9]+$/)
expect(ids[1]).toBe("kept")
expect(out.xml).toContain('name="One"')
})
it("rejects a shape that uses a root cell id", () => {
// It would clash with the added root cell "1" and be renamed,
// which breaks the edges that point to it
+81 -2
View File
@@ -31,7 +31,9 @@ interface PendingExport {
const realSetTimeout = globalThis.setTimeout
function open(options: { currentPageId?: string | null } = {}) {
function open(
options: { currentPageId?: string | null; isHidden?: () => boolean } = {},
) {
const calls: Call[] = []
const fetchStub = vi.fn(
(url: string, init?: RequestInit) =>
@@ -63,10 +65,12 @@ function open(options: { currentPageId?: string | null } = {}) {
const canvas: {
xml: string
pageId: string | null
loadPending: boolean
selection: SelectionAnswer
} = {
xml: "",
pageId: options.currentPageId ?? null,
loadPending: false,
selection: { pageId: "p1", pageName: "Page-1", cells: [] },
}
const sync = createMcpSync({
@@ -95,11 +99,13 @@ function open(options: { currentPageId?: string | null } = {}) {
}),
currentXml: () => canvas.xml,
currentPageId: () => canvas.pageId,
loadPending: () => canvas.loadPending,
readSelection: () => canvas.selection,
},
onNotice: (notice) => notices.push(notice),
onStatus: (status) => statuses.push(status),
onStateRecreated: () => recreated++,
isHidden: options.isHidden,
})
const settle = () => new Promise((r) => realSetTimeout(r, 0))
const next = (method: string, prefix = "/api/") => {
@@ -155,7 +161,9 @@ const state = (
})
/** A tab in step with state S1 at version 2, showing diagram A, draw.io ready */
async function inStep(options: { currentPageId?: string | null } = {}) {
async function inStep(
options: { currentPageId?: string | null; isHidden?: () => boolean } = {},
) {
const t = open(options)
t.sync.setReady(true)
const poll = t.sync.poll()
@@ -958,6 +966,77 @@ describe("MCP sync selection requests (get_selection)", () => {
})
})
it("answers again at the next poll when the answer was not delivered", async () => {
const t = await inStep()
t.canvas.selection = picked
let poll = t.sync.poll()
t.next("GET").answer(selectionState())
await poll
t.next("POST").fail()
await t.settle()
// The server still shows the request: answered again
poll = t.sync.poll()
t.next("GET").answer(selectionState())
await poll
const retry = t.next("POST")
expect(retry.body.selectionId).toBe("sel-1")
retry.answer({ status: 500, body: {} })
await t.settle()
poll = t.sync.poll()
t.next("GET").answer(selectionState())
await poll
t.next("POST").answer({ status: 200, body: { success: true } })
await t.settle()
// Delivered: not again
poll = t.sync.poll()
t.next("GET").answer(selectionState())
await poll
expect(t.posts()).toHaveLength(0)
})
it("waits for a full load to reach the editor before reading the selection", async () => {
// The poll that brings the request may also bring a new document,
// loaded in full: until draw.io reports it, the editor shows the old
// one, whose cells and pages the answer would name
const t = await inStep()
t.canvas.selection = picked
t.canvas.loadPending = true
let poll = t.sync.poll()
t.next("GET").answer(
state("S1", 3, "<mxfile>B</mxfile>", { selectionId: "sel-1" }),
)
await poll
expect(t.loads.at(-1)?.xml).toBe("<mxfile>B</mxfile>")
expect(t.posts()).toHaveLength(0)
t.canvas.loadPending = false
poll = t.sync.poll()
t.next("GET").answer(
state("S1", 3, "<mxfile>B</mxfile>", { selectionId: "sel-1" }),
)
await poll
expect(t.next("POST").body).toMatchObject({
selectionId: "sel-1",
selection: picked,
})
})
it("lets a tab in front answer first when this one is hidden", async () => {
// The same session open twice: the hidden tab's selection is not
// the user's; it answers a poll later, in case it is the only tab
const t = await inStep({ isHidden: () => true })
t.canvas.selection = picked
const poll = t.sync.poll()
t.next("GET").answer(selectionState())
await poll
expect(t.posts()).toHaveLength(0)
await vi.advanceTimersByTimeAsync(2500)
expect(t.next("POST").body).toEqual({
sessionId: "mcp-test",
selectionId: "sel-1",
selection: picked,
})
})
it("never answers while a projection is on screen", async () => {
const t = await inStep()
t.canvas.selection = picked