diff --git a/contexts/diagram-context.tsx b/contexts/diagram-context.tsx index a59e542b..902b448a 100644 --- a/contexts/diagram-context.tsx +++ b/contexts/diagram-context.tsx @@ -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, diff --git a/lib/drawio/editor-bridge.ts b/lib/drawio/editor-bridge.ts index 157b5a86..2936cfcd 100644 --- a/lib/drawio/editor-bridge.ts +++ b/lib/drawio/editor-bridge.ts @@ -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 diff --git a/packages/mcp-server/shell/mcp-sync-core.ts b/packages/mcp-server/shell/mcp-sync-core.ts index dca5bb94..62a7c032 100644 --- a/packages/mcp-server/shell/mcp-sync-core.ts +++ b/packages/mcp-server/shell/mcp-sync-core.ts @@ -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") diff --git a/packages/mcp-server/shell/use-mcp-sync.ts b/packages/mcp-server/shell/use-mcp-sync.ts index fc4c681e..8bf4bb22 100644 --- a/packages/mcp-server/shell/use-mcp-sync.ts +++ b/packages/mcp-server/shell/use-mcp-sync.ts @@ -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) diff --git a/packages/mcp-server/src/http-server.ts b/packages/mcp-server/src/http-server.ts index b23d6bcd..df162dcc 100644 --- a/packages/mcp-server/src/http-server.ts +++ b/packages/mcp-server/src/http-server.ts @@ -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 = 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 { + 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 { "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", ) diff --git a/packages/mcp-server/src/index.ts b/packages/mcp-server/src/index.ts index 1cdd09be..8a96779d 100644 --- a/packages/mcp-server/src/index.ts +++ b/packages/mcp-server/src/index.ts @@ -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: [ diff --git a/packages/mcp-server/src/new-diagram.ts b/packages/mcp-server/src/new-diagram.ts index 68617ed7..5ba803bf 100644 --- a/packages/mcp-server/src/new-diagram.ts +++ b/packages/mcp-server/src/new-diagram.ts @@ -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 , or an with one or more children.", } } + // A 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 } } diff --git a/packages/mcp-server/src/selection.ts b/packages/mcp-server/src/selection.ts index c3ab3d77..db098b37 100644 --- a/packages/mcp-server/src/selection.ts +++ b/packages/mcp-server/src/selection.ts @@ -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}")` : "" diff --git a/packages/mcp-server/tests/http-server.test.ts b/packages/mcp-server/tests/http-server.test.ts index ab4bc4f5..f69f41cd 100644 --- a/packages/mcp-server/tests/http-server.test.ts +++ b/packages/mcp-server/tests/http-server.test.ts @@ -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, "x") + // 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, "x") + 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, "x") + 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) } diff --git a/packages/mcp-server/tests/selection.test.ts b/packages/mcp-server/tests/selection.test.ts index d24fc664..739cac9f 100644 --- a/packages/mcp-server/tests/selection.test.ts +++ b/packages/mcp-server/tests/selection.test.ts @@ -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( diff --git a/packages/mcp-server/tests/wrap-cells.test.ts b/packages/mcp-server/tests/wrap-cells.test.ts index 176dc2a7..8db74a92 100644 --- a/packages/mcp-server/tests/wrap-cells.test.ts +++ b/packages/mcp-server/tests/wrap-cells.test.ts @@ -146,6 +146,24 @@ describe("reservedIdError", () => { }) describe("prepareNewDiagram", () => { + it("gives every 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( + ``, + ) + expect(out.ok).toBe(true) + if (!out.ok) return + const ids = [...out.xml.matchAll(/]*\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 diff --git a/tests/unit/mcp-sync-core.test.ts b/tests/unit/mcp-sync-core.test.ts index f0fa60bd..74f6da64 100644 --- a/tests/unit/mcp-sync-core.test.ts +++ b/tests/unit/mcp-sync-core.test.ts @@ -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, "B", { selectionId: "sel-1" }), + ) + await poll + expect(t.loads.at(-1)?.xml).toBe("B") + expect(t.posts()).toHaveLength(0) + t.canvas.loadPending = false + poll = t.sync.poll() + t.next("GET").answer( + state("S1", 3, "B", { 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