From 5c7613ea09f9695619d49b21c61e61cf6b249b3d Mon Sep 17 00:00:00 2001 From: "dayuan.jiang" Date: Sat, 3 Oct 2026 17:45:25 +0900 Subject: [PATCH] fix(diagram): fix autosave staleness and XML repair corrupting valid diagrams - Autosave guard reads refs, so edits after a theme or dark mode switch are kept - Duplicate-id check and rename run per page; repair loop no longer quadratic - autoFixXml no longer breaks style values, rich text " or single-line cells - extractCompleteMxCells keeps the cell after a self-closing cell - Better truncation detection; object/UserObject wrapped cells are editable - Exports for thumbnail, PNG and save are routed by tag instead of a shared resolver - History stores the full document; storage errors are reported, no auto-deletion of chats - IndexedDB connection reopens after errors; focus refresh throttled - Keep ?session= on locale redirect, map zh-Hant to zh-tw for draw.io --- app/[lang]/page.tsx | 29 +- contexts/diagram-context.tsx | 145 +++++---- hooks/use-session-manager.ts | 36 ++- lib/i18n/dictionaries/en.json | 3 +- lib/i18n/dictionaries/ja.json | 3 +- lib/i18n/dictionaries/zh-Hant.json | 3 +- lib/i18n/dictionaries/zh.json | 3 +- lib/session-storage.ts | 54 ++-- lib/utils.ts | 455 +++++++++++++++++------------ tests/unit/utils.test.ts | 186 +++++++++++- 10 files changed, 602 insertions(+), 315 deletions(-) diff --git a/app/[lang]/page.tsx b/app/[lang]/page.tsx index b44c9fef..9c32a2b2 100644 --- a/app/[lang]/page.tsx +++ b/app/[lang]/page.tsx @@ -37,7 +37,6 @@ export default function Home() { ) const chatPanelRef = useRef(null) - const isMobileRef = useRef(false) // Load preferences from localStorage after mount useEffect(() => { @@ -48,7 +47,9 @@ export default function Home() { const currentLocale = pathParts[0] if (currentLocale !== savedLocale) { pathParts[0] = savedLocale - router.replace(`/${pathParts.join("/")}`) + // Keep the query (e.g. ?session=) and hash + const { search, hash } = window.location + router.replace(`/${pathParts.join("/")}${search}${hash}`) return // Wait for redirect } } @@ -106,27 +107,17 @@ export default function Home() { resetDrawioReady() } - // Check mobile - reset draw.io before crossing breakpoint - const isInitialRenderRef = useRef(true) + // Check mobile. The draw.io iframe is not remounted when crossing the + // breakpoint (only the chat panel is), so its ready state stays as is. useEffect(() => { const checkMobile = () => { - const newIsMobile = window.innerWidth < 768 - if ( - !isInitialRenderRef.current && - newIsMobile !== isMobileRef.current - ) { - setIsDrawioReady(false) - resetDrawioReady() - } - isMobileRef.current = newIsMobile - isInitialRenderRef.current = false - setIsMobile(newIsMobile) + setIsMobile(window.innerWidth < 768) } checkMobile() window.addEventListener("resize", checkMobile) return () => window.removeEventListener("resize", checkMobile) - }, [resetDrawioReady]) + }, []) const toggleChatPanel = () => { const panel = chatPanelRef.current @@ -193,7 +184,11 @@ export default function Home() { noExitBtn: true, dark: darkMode || drawioUi === "dark", - lang: currentLang, + // draw.io names Traditional Chinese "zh-tw" + lang: + currentLang === "zh-Hant" + ? "zh-tw" + : currentLang, // Enable offline mode in Electron to disable external service calls ...(isElectron && { offline: true, diff --git a/contexts/diagram-context.tsx b/contexts/diagram-context.tsx index 6a139a08..e4071012 100644 --- a/contexts/diagram-context.tsx +++ b/contexts/diagram-context.tsx @@ -1,7 +1,7 @@ "use client" import type React from "react" -import { createContext, useContext, useEffect, useRef, useState } from "react" +import { createContext, useContext, useRef, useState } from "react" import type { DrawIoEmbedRef, EventExport } from "react-drawio" import { toast } from "sonner" import type { ExportFormat } from "@/components/save-dialog" @@ -42,6 +42,12 @@ interface DiagramContextType { const DiagramContext = createContext(undefined) +// Exports for thumbnails, validation PNGs and file saves carry a tag in the +// request's `message` field. draw.io echoes the request back in the export +// event, so each result reaches its own caller; untagged exports (chat-panel's +// onFetchChart) resolve resolverRef. +type ExportTag = "thumbnail" | "validation" + export function DiagramProvider({ children }: { children: React.ReactNode }) { const [chartXML, setChartXML] = useState("") const [latestSvg, setLatestSvg] = useState("") @@ -53,8 +59,10 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { const hasCalledOnLoadRef = useRef(false) const drawioRef = useRef(null) const resolverRef = useRef<((value: string) => void) | null>(null) - // Resolver for PNG export (used for VLM validation) - const pngResolverRef = useRef<((value: string) => void) | null>(null) + // Pending thumbnail and validation PNG exports, keyed by their export tag + const taggedResolversRef = useRef< + Partial void>> + >({}) // Track if we're expecting an export for history (user-initiated) const expectHistoryExportRef = useRef(false) // Track latest chartXML for restoration after remount @@ -76,10 +84,12 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { setIsDrawioReady(false) } - // Keep chartXMLRef in sync with state for restoration after remount - useEffect(() => { - chartXMLRef.current = chartXML - }, [chartXML]) + // Update chartXML and its ref together, so callbacks that read the ref + // (export handler, autosave) see the new value right away + const updateChartXML = (xml: string) => { + chartXMLRef.current = xml + setChartXML(xml) + } // Track if we're expecting an export for file save (stores raw export data) const saveResolverRef = useRef<{ @@ -106,64 +116,52 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { } } + // Export with a tag in `message` (draw.io echoes it back in the export + // event) and wait for that result. Resolves to null on timeout, which is + // expected occasionally. + const requestTaggedExport = ( + tag: ExportTag, + format: "xmlsvg" | "png", + timeoutMs: number, + ) => + new Promise((resolve) => { + const finish = (value: string | null) => { + clearTimeout(timer) + if (taggedResolversRef.current[tag] === finish) { + delete taggedResolversRef.current[tag] + } + resolve(value) + } + const timer = setTimeout(() => finish(null), timeoutMs) + taggedResolversRef.current[tag] = finish + drawioRef.current?.exportDiagram({ format, message: tag }) + }) + // Get current diagram as SVG for thumbnail (used by session storage) const getThumbnailSvg = async (): Promise => { if (!drawioRef.current) return null // Don't export if diagram is empty - if (!isRealDiagram(chartXML)) return null - - try { - const svgData = await Promise.race([ - new Promise((resolve) => { - resolverRef.current = resolve - drawioRef.current?.exportDiagram({ format: "xmlsvg" }) - }), - new Promise((_, reject) => - setTimeout(() => reject(new Error("Export timeout")), 3000), - ), - ]) + if (!isRealDiagram(chartXMLRef.current)) return null + // xmlsvg exports return an SVG data URL + const svgData = await requestTaggedExport("thumbnail", "xmlsvg", 3000) + if (svgData?.startsWith("data:image/svg")) { // Update latestSvg so it's available for future saves - if (svgData?.includes(" => { if (!drawioRef.current) return null // Don't export if diagram is empty - if (!isRealDiagram(chartXML)) return null + if (!isRealDiagram(chartXMLRef.current)) return null - try { - const pngData = await Promise.race([ - new Promise((resolve) => { - pngResolverRef.current = resolve - drawioRef.current?.exportDiagram({ format: "png" }) - }), - new Promise((_, reject) => - setTimeout( - () => reject(new Error("PNG export timeout")), - 5000, - ), - ), - ]) - - // PNG data should be a base64 data URL - if (pngData?.startsWith("data:image/png")) { - return pngData - } - return null - } catch { - // Timeout is expected occasionally - don't log as error - return null - } + const pngData = await requestTaggedExport("validation", "png", 5000) + // PNG data should be a base64 data URL + return pngData?.startsWith("data:image/png") ? pngData : null } const loadDiagram = ( @@ -193,7 +191,7 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { } // Keep chartXML in sync even when diagrams are injected (e.g., display_diagram tool) - setChartXML(xmlToLoad) + updateChartXML(xmlToLoad) if (drawioRef.current) { drawioRef.current.load({ @@ -205,24 +203,17 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { } const handleDiagramExport = (data: EventExport) => { - // Handle PNG export for VLM validation - if (pngResolverRef.current && data.data?.startsWith("data:image/png")) { - pngResolverRef.current(data.data) - pngResolverRef.current = null + // Tagged exports (thumbnail, validation PNG, file save) go only to + // their own caller, so they never take the result meant for resolverRef + const tag = data.message?.message + if (tag === "thumbnail" || tag === "validation") { + taggedResolversRef.current[tag]?.(data.data) return } - - // Handle save to file if requested (process raw data before extraction) - if (saveResolverRef.current.resolver) { - const format = saveResolverRef.current.format - saveResolverRef.current.resolver(data.data, data.xml) + if (tag === "save") { + saveResolverRef.current.resolver?.(data.data, data.xml) saveResolverRef.current = { resolver: null, format: null } - // For non-xmlsvg formats, skip XML extraction as it will fail - // Only drawio (which uses xmlsvg internally) has the content attribute - // xmlsvg is saved directly as SVG file, no need for extraction - if (format === "png" || format === "svg" || format === "xmlsvg") { - return - } + return } // Don't write chartXML here: exports don't change the diagram, and @@ -236,12 +227,15 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { // Limit to 20 entries to prevent memory leaks during long sessions const MAX_HISTORY_SIZE = 20 if (expectHistoryExportRef.current) { + // Store the full multi-page document (extractedXML is only the + // first page), so restoring a version keeps every page + const historyXml = chartXMLRef.current || extractedXML setDiagramHistory((prev) => { const newHistory = [ ...prev, { svg: data.data, - xml: extractedXML, + xml: historyXml, }, ] // Keep only the last MAX_HISTORY_SIZE entries (circular buffer) @@ -256,14 +250,16 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { } } + // react-drawio registers this callback once per iframe mount, so it must + // read refs: state captured in its closure would stay stale after a remount const handleDiagramAutoSave = (data: { xml?: string }) => { if (!data?.xml) return - // Don't overwrite a pending restore - if we have a real diagram in state - // but DrawIO isn't ready yet, it means we're waiting to restore - if (!isDrawioReady && isRealDiagram(chartXML)) { + // Don't overwrite a pending restore - if we have a real diagram but + // DrawIO hasn't loaded yet, it means we're waiting to restore + if (!hasCalledOnLoadRef.current && isRealDiagram(chartXMLRef.current)) { return } - setChartXML(data.xml) + updateChartXML(data.xml) } const clearDiagram = () => { @@ -365,7 +361,10 @@ export function DiagramProvider({ children }: { children: React.ReactNode }) { } // Export diagram - callback will be handled in handleDiagramExport - drawioRef.current.exportDiagram({ format: drawioFormat }) + drawioRef.current.exportDiagram({ + format: drawioFormat, + message: "save", + }) } // Log save event to Langfuse (just flags the trace, doesn't send content) diff --git a/hooks/use-session-manager.ts b/hooks/use-session-manager.ts index 9243e2c3..75c11b78 100644 --- a/hooks/use-session-manager.ts +++ b/hooks/use-session-manager.ts @@ -1,6 +1,8 @@ "use client" import { useCallback, useEffect, useRef, useState } from "react" +import { toast } from "sonner" +import { useDictionary } from "@/hooks/use-dictionary" import { type ChatSession, createEmptySession, @@ -44,6 +46,15 @@ export interface UseSessionManagerReturn { clearCurrentSession: () => void } +// Reading the session list loads every stored session in full, and window +// focus also fires each time the user clicks back from the draw.io iframe +const FOCUS_REFRESH_INTERVAL_MS = 30_000 + +function notifySaveFailed(message: string) { + // Same id, so repeated failures update one toast instead of stacking + toast.error(message, { id: "session-save-failed", duration: 8000 }) +} + interface UseSessionManagerOptions { /** Session ID from URL param - if provided, load this session; if null, start blank */ initialSessionId?: string | null @@ -53,6 +64,7 @@ export function useSessionManager( options: UseSessionManagerOptions = {}, ): UseSessionManagerReturn { const { initialSessionId } = options + const dict = useDictionary() const [sessions, setSessions] = useState([]) const [currentSessionId, setCurrentSessionId] = useState( null, @@ -163,9 +175,15 @@ export function useSessionManager( handleSessionIdChange() }, [initialSessionId, isAvailable]) - // Refresh sessions on window focus (multi-tab sync) + // Refresh sessions on window focus (multi-tab sync), at most once per interval + const lastFocusRefreshRef = useRef(0) useEffect(() => { const handleFocus = () => { + const now = Date.now() + if (now - lastFocusRefreshRef.current < FOCUS_REFRESH_INTERVAL_MS) { + return + } + lastFocusRefreshRef.current = now refreshSessions() } window.addEventListener("focus", handleFocus) @@ -238,6 +256,8 @@ export function useSessionManager( ) { return } + // Nothing can be stored without IndexedDB + if (!isIndexedDBAvailable()) return if (!currentSession) { // Create a new session if none exists @@ -250,7 +270,12 @@ export function useSessionManager( diagramHistory: data.diagramHistory, title: extractTitle(data.messages), } - await saveSession(newSession) + // Without a stored session, keep no session id (it would end + // up in the URL and point to nothing after a reload) + if (!(await saveSession(newSession))) { + notifySaveFailed(dict.errors.sessionSaveFailed) + return + } await enforceSessionLimit() setCurrentSession(newSession) setCurrentSessionId(newSession.id) @@ -277,7 +302,10 @@ export function useSessionManager( : currentSession.title, } - await saveSession(updatedSession) + if (!(await saveSession(updatedSession))) { + notifySaveFailed(dict.errors.sessionSaveFailed) + return + } setCurrentSession(updatedSession) // Update sessions list metadata @@ -298,7 +326,7 @@ export function useSessionManager( ), ) }, - [currentSession, currentSessionId, refreshSessions], + [currentSession, currentSessionId, refreshSessions, dict], ) // Clear current session state (for starting fresh without loading another session) diff --git a/lib/i18n/dictionaries/en.json b/lib/i18n/dictionaries/en.json index 61cf73fc..77ec3e0a 100644 --- a/lib/i18n/dictionaries/en.json +++ b/lib/i18n/dictionaries/en.json @@ -188,7 +188,8 @@ "failedToExport": "Error fetching chart data", "failedToLoadExample": "Error loading example image", "failedToRecordFeedback": "Failed to record your feedback. Please try again.", - "storageUpdateFailed": "Chat cleared but browser storage could not be updated" + "storageUpdateFailed": "Chat cleared but browser storage could not be updated", + "sessionSaveFailed": "Could not save this chat. Browser storage may be full: delete old chats from history and try again." }, "quota": { "dailyLimit": "Daily Quota Reached", diff --git a/lib/i18n/dictionaries/ja.json b/lib/i18n/dictionaries/ja.json index c4393bb6..71018e95 100644 --- a/lib/i18n/dictionaries/ja.json +++ b/lib/i18n/dictionaries/ja.json @@ -188,7 +188,8 @@ "failedToExport": "チャートデータの取得エラー", "failedToLoadExample": "例の画像の読み込みエラー", "failedToRecordFeedback": "フィードバックの記録に失敗しました。もう一度お試しください。", - "storageUpdateFailed": "チャットはクリアされましたが、ブラウザストレージを更新できませんでした" + "storageUpdateFailed": "チャットはクリアされましたが、ブラウザストレージを更新できませんでした", + "sessionSaveFailed": "このチャットを保存できませんでした。ブラウザのストレージがいっぱいの可能性があります。履歴から古いチャットを削除して、もう一度お試しください。" }, "quota": { "dailyLimit": "1日の割当量に達しました", diff --git a/lib/i18n/dictionaries/zh-Hant.json b/lib/i18n/dictionaries/zh-Hant.json index 5de362c2..c6e78986 100644 --- a/lib/i18n/dictionaries/zh-Hant.json +++ b/lib/i18n/dictionaries/zh-Hant.json @@ -188,7 +188,8 @@ "failedToExport": "取得圖表資料時出錯", "failedToLoadExample": "載入範例圖片時出錯", "failedToRecordFeedback": "記錄您的回饋失敗。請重試。", - "storageUpdateFailed": "聊天已清除，但無法更新瀏覽器儲存空間" + "storageUpdateFailed": "聊天已清除，但無法更新瀏覽器儲存空間", + "sessionSaveFailed": "無法儲存這個對話。瀏覽器儲存空間可能已滿，請在歷史紀錄裡刪除舊對話後重試。" }, "quota": { "dailyLimit": "已達每日配額", diff --git a/lib/i18n/dictionaries/zh.json b/lib/i18n/dictionaries/zh.json index f46601e9..ae549068 100644 --- a/lib/i18n/dictionaries/zh.json +++ b/lib/i18n/dictionaries/zh.json @@ -188,7 +188,8 @@ "failedToExport": "获取图表数据时出错", "failedToLoadExample": "加载示例图片时出错", "failedToRecordFeedback": "记录您的反馈失败。请重试。", - "storageUpdateFailed": "聊天已清除，但无法更新浏览器存储" + "storageUpdateFailed": "聊天已清除，但无法更新浏览器存储", + "sessionSaveFailed": "无法保存这个对话。浏览器存储空间可能已满，请在历史记录里删除旧对话后重试。" }, "quota": { "dailyLimit": "已达每日配额", diff --git a/lib/session-storage.ts b/lib/session-storage.ts index 8450375c..b693cbde 100644 --- a/lib/session-storage.ts +++ b/lib/session-storage.ts @@ -1,5 +1,6 @@ import { type DBSchema, type IDBPDatabase, openDB } from "idb" import { nanoid } from "nanoid" +import { toast } from "sonner" import type { Template } from "./template-storage" // Constants @@ -61,6 +62,7 @@ let dbPromise: Promise> | null = null async function getDB(): Promise> { if (!dbPromise) { + // A failed or lost connection is not cached: the next call reopens it dbPromise = openDB(DB_NAME, DB_VERSION, { upgrade(db, oldVersion) { if (oldVersion < 1) { @@ -88,6 +90,28 @@ async function getDB(): Promise> { } } }, + blocked() { + // An older tab keeps the DB open, so the upgrade has to wait + toast.warning( + "Please close other tabs of this app to finish updating chat storage.", + { id: "idb-upgrade-blocked", duration: 10000 }, + ) + }, + blocking(_currentVersion, _blockedVersion, event) { + // Another tab needs to upgrade the DB: close our connection so + // it is not stuck, and reopen on the next call + const db = event.target as IDBDatabase + db.close() + dbPromise = null + }, + terminated() { + // The browser closed the connection (e.g. Safari after a long + // time in the background) + dbPromise = null + }, + }).catch((error) => { + dbPromise = null + throw error }) } return dbPromise @@ -145,6 +169,8 @@ export async function getSession(id: string): Promise { } } +// Returns false on failure (e.g. storage quota exceeded). Other sessions are +// never deleted automatically; the caller tells the user instead. export async function saveSession(session: ChatSession): Promise { if (!isIndexedDBAvailable()) return false try { @@ -152,29 +178,11 @@ export async function saveSession(session: ChatSession): Promise { await db.put(STORE_NAME, session) return true } catch (error) { - // Handle quota exceeded - if ( - error instanceof DOMException && - error.name === "QuotaExceededError" - ) { - console.warn("Storage quota exceeded, deleting oldest session...") - await deleteOldestSession() - // Retry once - try { - const db = await getDB() - await db.put(STORE_NAME, session) - return true - } catch (retryError) { - console.error( - "Failed to save session after cleanup:", - retryError, - ) - return false - } - } else { - console.error("Failed to save session:", error) - return false - } + console.error("Failed to save session:", error) + // Reopen the connection next time in case it was lost (Safari reports + // "Connection to Indexed Database server lost" without closing it) + dbPromise = null + return false } } diff --git a/lib/utils.ts b/lib/utils.ts index 2c1f8b43..b51ad5dd 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -76,6 +76,17 @@ export function isMxCellXmlComplete(xml: string | undefined | null): boolean { // No valid ending found at all if (lastValidEnd === -1) return false + // If the last mxCell has no after it, it must be self-closing. + // Otherwise the trailing "/>" belongs to a child such as + // and the output was cut off before the cell was closed. + const lastCellStart = trimmed.lastIndexOf(" lastMxCellClose && + !/^/.test(trimmed.slice(lastCellStart)) + ) { + return false + } + // Check what comes after the last valid ending // For />: add 2 chars, for : add 9 chars const endOffset = lastMxCellClose > lastSelfClose ? 9 : 2 @@ -95,36 +106,12 @@ export function isMxCellXmlComplete(xml: string | undefined | null): boolean { export function extractCompleteMxCells(xml: string | undefined | null): string { if (!xml) return "" - const completeCells: Array<{ index: number; text: string }> = [] + // Match self-closing or ..., in document order. + // The lazy [^>]*? tries "/>" first, so a self-closing cell never swallows + // the following cells up to the next . + const cellPattern = /]*?(?:\/>|>[\s\S]*?<\/mxCell>)/g - // Match self-closing mxCell tags: - // Also match mxCell with nested mxGeometry: ... - const selfClosingPattern = /]*\/>/g - const nestedPattern = /]*>[\s\S]*?<\/mxCell>/g - - // Find all self-closing mxCell elements - let match: RegExpExecArray | null - while ((match = selfClosingPattern.exec(xml)) !== null) { - completeCells.push({ index: match.index, text: match[0] }) - } - - // Find all mxCell elements with nested content (like mxGeometry) - while ((match = nestedPattern.exec(xml)) !== null) { - completeCells.push({ index: match.index, text: match[0] }) - } - - // Sort by position to maintain order - completeCells.sort((a, b) => a.index - b.index) - - // Remove duplicates (a self-closing match might overlap with nested match) - const seen = new Set() - const uniqueCells = completeCells.filter((cell) => { - if (seen.has(cell.index)) return false - seen.add(cell.index) - return true - }) - - return uniqueCells.map((c) => c.text).join("\n") + return (xml.match(cellPattern) || []).join("\n") } // ============================================================================ @@ -487,6 +474,31 @@ export interface ApplyOperationsResult { errors: OperationError[] } +/** + * draw.io wraps cells that have links, tooltips or custom data in + * /, and the wrapper carries the id instead of the mxCell. + */ +function getCellWrapper(cell: Element): Element | null { + const parent = cell.parentElement + return parent?.tagName === "object" || parent?.tagName === "UserObject" + ? parent + : null +} + +/** Id of a cell, read from its wrapper when the mxCell has none */ +function getCellId(cell: Element): string | null { + return ( + cell.getAttribute("id") || + getCellWrapper(cell)?.getAttribute("id") || + null + ) +} + +/** Element to replace or remove for a cell (the wrapper if there is one) */ +function getCellNode(cell: Element): Element { + return getCellWrapper(cell) || cell +} + /** * Apply diagram operations (update/add/delete) using ID-based lookup. * This replaces the text-matching approach with direct DOM manipulation. @@ -535,12 +547,14 @@ export function applyDiagramOperations( } } - // Build a map of cell IDs to elements + // Build a map of cell IDs to elements (wrapper elements for wrapped cells) const cellMap = new Map() root.querySelectorAll("mxCell").forEach((cell) => { - const id = cell.getAttribute("id") - if (id) cellMap.set(id, cell) + const id = getCellId(cell) + if (id) cellMap.set(id, getCellNode(cell)) }) + // Cells removed by delete operations in this batch + const deletedIds = new Set() // Process each operation for (const op of operations) { @@ -580,7 +594,7 @@ export function applyDiagramOperations( } // Validate ID matches - const newCellId = newCell.getAttribute("id") + const newCellId = getCellId(newCell) if (newCellId !== op.cell_id) { errors.push({ type: "update", @@ -590,8 +604,8 @@ export function applyDiagramOperations( continue } - // Import and replace the node - const importedNode = doc.importNode(newCell, true) + // Import and replace the node (with its wrapper, if any) + const importedNode = doc.importNode(getCellNode(newCell), true) existingCell.parentNode?.replaceChild(importedNode, existingCell) // Update the map with the new element @@ -632,7 +646,7 @@ export function applyDiagramOperations( } // Validate ID matches - const newCellId = newCell.getAttribute("id") + const newCellId = getCellId(newCell) if (newCellId !== op.cell_id) { errors.push({ type: "add", @@ -642,8 +656,8 @@ export function applyDiagramOperations( continue } - // Import and append the node - const importedNode = doc.importNode(newCell, true) + // Import and append the node (with its wrapper, if any) + const importedNode = doc.importNode(getCellNode(newCell), true) root.appendChild(importedNode) // Add to map @@ -661,8 +675,15 @@ export function applyDiagramOperations( const existingCell = cellMap.get(op.cell_id) if (!existingCell) { - // Cell not found - might have been cascade-deleted by a previous operation - // Skip silently instead of erroring (AI may redundantly list children/edges) + // Cells cascade-deleted earlier in this batch are skipped silently + // (AI may redundantly list children/edges) + if (!deletedIds.has(op.cell_id)) { + errors.push({ + type: "delete", + cellId: op.cell_id, + message: `Cell with id="${op.cell_id}" not found`, + }) + } continue } @@ -679,7 +700,7 @@ export function applyDiagramOperations( `mxCell[parent="${cellId}"]`, ) children.forEach((child) => { - const childId = child.getAttribute("id") + const childId = getCellId(child) if (childId && childId !== "0" && childId !== "1") { collectDescendants(childId) } @@ -696,7 +717,7 @@ export function applyDiagramOperations( `mxCell[source="${cellId}"], mxCell[target="${cellId}"]`, ) referencingEdges.forEach((edge) => { - const edgeId = edge.getAttribute("id") + const edgeId = getCellId(edge) // Protect root cells from being added via edge references if (edgeId && edgeId !== "0" && edgeId !== "1") { // Recurse to collect edge's children (like labels) @@ -718,6 +739,7 @@ export function applyDiagramOperations( if (cell) { cell.parentNode?.removeChild(cell) cellMap.delete(cellId) + deletedIds.add(cellId) } } } @@ -758,24 +780,89 @@ function checkDuplicateAttributes(xml: string): string | null { return null } -/** Check for duplicate IDs in XML */ -function checkDuplicateIds(xml: string): string | null { - const idPattern = /\bid\s*=\s*["']([^"']+)["']/gi +/** Matches one page of a document (the last one may be unclosed) */ +const PAGE_PATTERN = /|$)/g + +const ID_ATTR_PATTERN = /\bid\s*=\s*["']([^"']+)["']/gi + +/** + * Split XML into pages. Ids only need to be unique within a page: every + * page of a multi-page document has its own root cells "0" and "1". + */ +function splitPages(xml: string): string[] { + return xml.match(PAGE_PATTERN) || [xml] +} + +/** Ids that appear more than once, with their counts */ +function findDuplicateIds(xml: string): Map { const ids = new Map() - let idMatch - while ((idMatch = idPattern.exec(xml)) !== null) { - const id = idMatch[1] - ids.set(id, (ids.get(id) || 0) + 1) + for (const match of xml.matchAll(ID_ATTR_PATTERN)) { + ids.set(match[1], (ids.get(match[1]) || 0) + 1) } - const duplicateIds = Array.from(ids.entries()) - .filter(([, count]) => count > 1) - .map(([id, count]) => `'${id}' (${count}x)`) - if (duplicateIds.length > 0) { - return `Invalid XML: Found duplicate ID(s): ${duplicateIds.slice(0, 3).join(", ")}. All id attributes must be unique.` + return new Map(Array.from(ids).filter(([, count]) => count > 1)) +} + +/** Check for duplicate IDs in XML (per page) */ +function checkDuplicateIds(xml: string): string | null { + for (const page of splitPages(xml)) { + const duplicateIds = Array.from(findDuplicateIds(page)).map( + ([id, count]) => `'${id}' (${count}x)`, + ) + if (duplicateIds.length > 0) { + return `Invalid XML: Found duplicate ID(s): ${duplicateIds.slice(0, 3).join(", ")}. All id attributes must be unique.` + } } return null } +/** Rename repeated ids in one page (keeps the first occurrence) */ +function renameDuplicateIds(xml: string): { xml: string; renamed: number } { + const duplicateIds = findDuplicateIds(xml) + if (duplicateIds.size === 0) return { xml, renamed: 0 } + + const idCounters = new Map() + const renamedXml = xml.replace(ID_ATTR_PATTERN, (match, id) => { + if (!duplicateIds.has(id)) return match + + const count = idCounters.get(id) || 0 + idCounters.set(id, count + 1) + + if (count === 0) return match // Keep first occurrence + + // Rename subsequent occurrences (the id sits just before the closing quote) + return `${match.slice(0, -id.length - 1)}${id}_dup${count}${match.slice(-1)}` + }) + return { xml: renamedXml, renamed: duplicateIds.size } +} + +/** + * Returns a function telling whether a position is inside a quoted attribute + * value. Positions must be queried in increasing order: the scan resumes where + * it stopped instead of starting over, which keeps large documents fast. + */ +function createQuoteTracker(str: string): (pos: number) => boolean { + let i = 0 + let inQuote = false + let quoteChar = "" + return (pos: number) => { + for (; i < pos && i < str.length; i++) { + const c = str[i] + if (inQuote) { + if (c === quoteChar) inQuote = false + } else if (c === '"' || c === "'") { + // Only quotes that follow "=" open an attribute value + let j = i - 1 + while (j >= 0 && /\s/.test(str[j])) j-- + if (j >= 0 && str[j] === "=") { + inQuote = true + quoteChar = c + } + } + } + return inQuote + } +} + /** Check for tag mismatches using parsed tags */ function checkTagMismatches(xml: string): string | null { const xmlWithoutComments = xml.replace(//g, "") @@ -1088,13 +1175,19 @@ export function autoFixXml(xml: string): { fixed: string; fixes: string[] } { // 3b. Fix malformed attribute values where " is used as delimiter instead of actual quotes // Pattern: attr="value" should become attr="value" (the " was meant to be the quote delimiter) // This commonly happens with dashPattern="1 1;" - const malformedQuotePattern = /(\s[a-zA-Z][a-zA-Z0-9_:-]*)="/ - if (malformedQuotePattern.test(fixed)) { - // Replace =" with =" and trailing " before next attribute or tag end with " - fixed = fixed.replace( - /(\s[a-zA-Z][a-zA-Z0-9_:-]*)="([^&]*?)"/g, - '$1="$2"', - ) + // Matches inside another attribute value are kept: rich text labels like + // value="<font color="#ff0000">..." are valid. + const isInsideQuotesFor3b = createQuoteTracker(fixed) + let malformedQuotesFixed = false + fixed = fixed.replace( + /(\s[a-zA-Z][a-zA-Z0-9_:-]*)="([^&]*?)"/g, + (match: string, attr: string, value: string, offset: number) => { + if (isInsideQuotesFor3b(offset)) return match + malformedQuotesFixed = true + return `${attr}="${value}"` + }, + ) + if (malformedQuotesFixed) { fixes.push( 'Fixed malformed attribute quotes (="..." to ="...")', ) @@ -1108,9 +1201,11 @@ export function autoFixXml(xml: string): { fixed: string; fixes: string[] } { } // 3d. Fix missing space between attributes like vertex="1"parent="1" - const missingSpacePattern = /("[^"]*")([a-zA-Z][a-zA-Z0-9_:-]*=)/g + // Requires name=" right after the quote, so the opening quote of a value + // such as style="rounded=1;..." is not mistaken for a closing one. + const missingSpacePattern = /"([a-zA-Z_:][\w:.-]*=")/g if (missingSpacePattern.test(fixed)) { - fixed = fixed.replace(/("[^"]*")([a-zA-Z][a-zA-Z0-9_:-]*=)/g, "$1 $2") + fixed = fixed.replace(missingSpacePattern, '" $1') fixes.push("Added missing space between attributes") } @@ -1240,32 +1335,13 @@ export function autoFixXml(xml: string): { fixed: string; fixes: string[] } { "mxPoint", "Array", "Object", + // Wrappers of cells with links, tooltips or custom data + "object", + "UserObject", "mxRectangle", ]) - // Helper: Check if a position is inside a quoted attribute value - // by counting unescaped quotes before that position - const isInsideQuotes = (str: string, pos: number): boolean => { - let inQuote = false - let quoteChar = "" - for (let i = 0; i < pos && i < str.length; i++) { - const c = str[i] - if (inQuote) { - if (c === quoteChar) inQuote = false - } else if (c === '"' || c === "'") { - // Check if this quote is part of an attribute (preceded by =) - // Look back for = sign - let j = i - 1 - while (j >= 0 && /\s/.test(str[j])) j-- - if (j >= 0 && str[j] === "=") { - inQuote = true - quoteChar = c - } - } - } - return inQuote - } - + const isInsideQuotesFor8c = createQuoteTracker(fixed) const foreignTagPattern = /<\/?([a-zA-Z][a-zA-Z0-9_]*)[^>]*>/g let foreignMatch const foreignTags = new Set() @@ -1280,7 +1356,7 @@ export function autoFixXml(xml: string): { fixed: string; fixes: string[] } { // Skip if this is a valid draw.io tag if (validDrawioTags.has(tagName)) continue // Skip if this tag is inside a quoted attribute value - if (isInsideQuotes(fixed, foreignMatch.index)) continue + if (isInsideQuotesFor8c(foreignMatch.index)) continue foreignTags.add(tagName) foreignTagPositions.push({ @@ -1352,10 +1428,11 @@ export function autoFixXml(xml: string): { fixed: string; fixes: string[] } { >() // Match full tags to detect self-closing by checking if ends with /> const fullTagPattern = /<(\/?[a-zA-Z][a-zA-Z0-9]*)[^>]*>/g + const isInsideQuotesFor10b = createQuoteTracker(fixed) let tagCountMatch while ((tagCountMatch = fullTagPattern.exec(fixed)) !== null) { // Skip tags inside quoted attribute values (e.g., value="Title") - if (isInsideQuotes(fixed, tagCountMatch.index)) continue + if (isInsideQuotesFor10b(tagCountMatch.index)) continue const fullMatch = tagCountMatch[0] // e.g., "" or "" const tagPart = tagCountMatch[1] // e.g., "mxCell" or "/mxCell" @@ -1445,125 +1522,112 @@ export function autoFixXml(xml: string): { fixed: string; fixes: string[] } { // 11. Fix nested mxCell by flattening // Pattern A: ...... (duplicate ID) // Pattern B: ...... (different ID - true nesting) - const lines = fixed.split("\n") - let newLines: string[] = [] - let nestedFixed = 0 - let extraClosingToRemove = 0 + // These passes work line by line and would break valid cells written on a + // single line, so each one runs only when cells are really nested. + if (checkNestedMxCells(fixed)) { + const lines = fixed.split("\n") + const newLines: string[] = [] + let nestedFixed = 0 + let extraClosingToRemove = 0 - // First pass: fix duplicate ID nesting (same as before) - for (let i = 0; i < lines.length; i++) { - const line = lines[i] - const nextLine = lines[i + 1] + // First pass: fix duplicate ID nesting (same as before) + for (let i = 0; i < lines.length; i++) { + const line = lines[i] + const nextLine = lines[i + 1] - // Check if current line and next line are both mxCell opening tags with same ID - if ( - nextLine && - /") && - !nextLine.includes("/>") - ) { - const id1 = line.match(/\bid\s*=\s*["']([^"']+)["']/)?.[1] - const id2 = nextLine.match(/\bid\s*=\s*["']([^"']+)["']/)?.[1] + // Check if current line and next line are both mxCell opening tags with same ID + if ( + nextLine && + /") && + !nextLine.includes("/>") + ) { + const id1 = line.match(/\bid\s*=\s*["']([^"']+)["']/)?.[1] + const id2 = nextLine.match(/\bid\s*=\s*["']([^"']+)["']/)?.[1] - if (id1 && id1 === id2) { - nestedFixed++ - extraClosingToRemove++ // Need to remove one later - continue // Skip this duplicate opening line + if (id1 && id1 === id2) { + nestedFixed++ + extraClosingToRemove++ // Need to remove one later + continue // Skip this duplicate opening line + } } - } - // Remove extra if we have pending removals - if (extraClosingToRemove > 0 && /^\s*<\/mxCell>\s*$/.test(line)) { - extraClosingToRemove-- - continue // Skip this closing tag - } - - newLines.push(line) - } - - if (nestedFixed > 0) { - fixed = newLines.join("\n") - fixes.push(`Flattened ${nestedFixed} duplicate-ID nested mxCell(s)`) - } - - // Second pass: fix true nesting (different IDs) - // Insert before nested child to close parent - const lines2 = fixed.split("\n") - newLines = [] - let trueNestedFixed = 0 - let cellDepth = 0 - let pendingCloseRemoval = 0 - - for (let i = 0; i < lines2.length; i++) { - const line = lines2[i] - const trimmed = line.trim() - - // Track mxCell depth - const isOpenCell = /") - const isCloseCell = trimmed === "" - - if (isOpenCell) { - if (cellDepth > 0) { - // Found nested cell - insert closing tag for parent before this line - const indent = line.match(/^(\s*)/)?.[1] || "" - newLines.push(indent + "") - trueNestedFixed++ - pendingCloseRemoval++ // Need to remove one later + // Remove extra if we have pending removals + if (extraClosingToRemove > 0 && /^\s*<\/mxCell>\s*$/.test(line)) { + extraClosingToRemove-- + continue // Skip this closing tag } - cellDepth = 1 // Reset to 1 since we just opened a new cell + newLines.push(line) - } else if (isCloseCell) { - if (pendingCloseRemoval > 0) { - pendingCloseRemoval-- - // Skip this extra closing tag + } + + if (nestedFixed > 0) { + fixed = newLines.join("\n") + fixes.push(`Flattened ${nestedFixed} duplicate-ID nested mxCell(s)`) + } + } + + if (checkNestedMxCells(fixed)) { + // Second pass: fix true nesting (different IDs) + // Insert before nested child to close parent + const lines2 = fixed.split("\n") + const newLines: string[] = [] + let trueNestedFixed = 0 + let cellDepth = 0 + let pendingCloseRemoval = 0 + + for (let i = 0; i < lines2.length; i++) { + const line = lines2[i] + const trimmed = line.trim() + + // Track mxCell depth + const isOpenCell = + /") + const isCloseCell = trimmed === "" + + if (isOpenCell) { + if (cellDepth > 0) { + // Found nested cell - insert closing tag for parent before this line + const indent = line.match(/^(\s*)/)?.[1] || "" + newLines.push(indent + "") + trueNestedFixed++ + pendingCloseRemoval++ // Need to remove one later + } + cellDepth = 1 // Reset to 1 since we just opened a new cell + newLines.push(line) + } else if (isCloseCell) { + if (pendingCloseRemoval > 0) { + pendingCloseRemoval-- + // Skip this extra closing tag + } else { + cellDepth = Math.max(0, cellDepth - 1) + newLines.push(line) + } } else { - cellDepth = Math.max(0, cellDepth - 1) newLines.push(line) } - } else { - newLines.push(line) + } + + if (trueNestedFixed > 0) { + fixed = newLines.join("\n") + fixes.push(`Fixed ${trueNestedFixed} true nested mxCell(s)`) } } - if (trueNestedFixed > 0) { - fixed = newLines.join("\n") - fixes.push(`Fixed ${trueNestedFixed} true nested mxCell(s)`) + // 12. Fix duplicate IDs by appending suffix, page by page (ids such as the + // root cells "0" and "1" legitimately repeat across pages) + let renamedIds = 0 + const renamePage = (page: string) => { + const { xml: renamed, renamed: count } = renameDuplicateIds(page) + renamedIds += count + return renamed } - - // 12. Fix duplicate IDs by appending suffix - const seenIds = new Map() - const duplicateIds: string[] = [] - - // First pass: find duplicates - const idPattern = /\bid\s*=\s*["']([^"']+)["']/gi - let idMatch - while ((idMatch = idPattern.exec(fixed)) !== null) { - const id = idMatch[1] - seenIds.set(id, (seenIds.get(id) || 0) + 1) - } - - // Find which IDs are duplicated - for (const [id, count] of seenIds) { - if (count > 1) duplicateIds.push(id) - } - - // Second pass: rename duplicates (keep first occurrence, rename others) - if (duplicateIds.length > 0) { - const idCounters = new Map() - fixed = fixed.replace(/\bid\s*=\s*["']([^"']+)["']/gi, (match, id) => { - if (!duplicateIds.includes(id)) return match - - const count = idCounters.get(id) || 0 - idCounters.set(id, count + 1) - - if (count === 0) return match // Keep first occurrence - - // Rename subsequent occurrences - const newId = `${id}_dup${count}` - return match.replace(id, newId) - }) - fixes.push(`Renamed ${duplicateIds.length} duplicate ID(s)`) + fixed = / 0) { + fixes.push(`Renamed ${renamedIds} duplicate ID(s)`) } // 9. Fix empty id attributes by generating unique IDs @@ -1673,6 +1737,11 @@ export function validateAndFixXml(xml: string): { } } +/** + * Decode an xmlsvg export (SVG data URL) into uncompressed diagram XML. + * Only the first page is returned; for the full multi-page document use the + * autosaved chartXML instead. + */ export function extractDiagramXML(xml_svg_string: string): string { try { // 1. Parse the SVG string (using built-in DOMParser in a browser-like environment) diff --git a/tests/unit/utils.test.ts b/tests/unit/utils.test.ts index 1176de39..59f72afc 100644 --- a/tests/unit/utils.test.ts +++ b/tests/unit/utils.test.ts @@ -1,5 +1,14 @@ import { describe, expect, it } from "vitest" -import { cn, isMxCellXmlComplete, wrapWithMxFile } from "@/lib/utils" +import { + applyDiagramOperations, + autoFixXml, + cn, + extractCompleteMxCells, + isMxCellXmlComplete, + validateAndFixXml, + validateMxCellStructure, + wrapWithMxFile, +} from "@/lib/utils" describe("isMxCellXmlComplete", () => { it("returns false for empty/null input", () => { @@ -33,6 +42,31 @@ describe("isMxCellXmlComplete", () => { expect(isMxCellXmlComplete(xml)).toBe(false) }) + it("returns false when output stops after a child of an open mxCell", () => { + const xml = ` + + + + ` + expect(isMxCellXmlComplete(xml)).toBe(false) + }) + + it("returns false when output stops after of an open mxCell", () => { + const xml = ` + + + ` + expect(isMxCellXmlComplete(xml)).toBe(false) + }) + + it("returns true for a self-closing last mxCell with > in its value", () => { + const xml = ` + + + ` + expect(isMxCellXmlComplete(xml)).toBe(true) + }) + it("returns true for multiple complete mxCells", () => { const xml = ` ` @@ -84,3 +118,153 @@ describe("cn (class name utility)", () => { expect(cn("text-red-500", "text-blue-500")).toBe("text-blue-500") }) }) + +describe("extractCompleteMxCells", () => { + it("keeps the cell right after self-closing root cells", () => { + const xml = `` + const ids = [ + ...extractCompleteMxCells(xml).matchAll(/ m[1]) + expect(ids).toEqual(["0", "1", "2", "3"]) + }) + + it("drops an incomplete trailing cell", () => { + const xml = `` + expect(extractCompleteMxCells(xml)).toBe( + '', + ) + }) +}) + +const page = (id: string, cells: string) => + `${cells}` + +describe("duplicate ids in multi-page documents", () => { + const shape = (id: string, value = "Box") => + `` + + it("accepts the same ids on different pages", () => { + const xml = `${page("p1", shape("2"))}${page("p2", shape("2"))}` + expect(validateMxCellStructure(xml)).toBeNull() + }) + + it("still reports duplicate ids within one page", () => { + const xml = `${page("p1", shape("2") + shape("2"))}${page("p2", "")}` + expect(validateMxCellStructure(xml)).toContain("duplicate ID") + }) + + it("does not rename the root cells of other pages when fixing", () => { + const xml = `${page("p1", shape("2", "R&D"))}${page("p2", shape("3"))}` + const result = validateAndFixXml(xml) + expect(result.valid).toBe(true) + expect(result.fixed).not.toContain("_dup") + expect(result.fixed).toContain("R&D") + }) + + it("renames a duplicate id within a page", () => { + const xml = `${page("p1", shape("d") + shape("d"))}` + const { fixed } = autoFixXml(xml) + expect(fixed).toContain(' { + it("does not insert a space at the start of style values", () => { + const xml = `` + const { fixed } = autoFixXml(xml) + expect(fixed).toContain('style="rounded=1;whiteSpace=wrap;"') + }) + + it("adds a missing space between attributes", () => { + const xml = `` + expect(autoFixXml(xml).fixed).toContain('vertex="1" parent="1"') + }) + + it("keeps " inside rich text labels", () => { + const label = "<font color="#ff0000">Hello</font>" + const xml = `` + const result = validateAndFixXml(xml) + expect(result.valid).toBe(true) + expect(result.fixed).toContain(`value="${label}"`) + }) + + it("fixes an attribute delimited by "", () => { + const xml = `` + expect(autoFixXml(xml).fixed).toContain('dashPattern="1 1;"') + }) + + it("keeps cells written on one line next to multi-line cells", () => { + const xml = ` + + + + + + + + + +` + const result = validateAndFixXml(xml) + expect(result.valid).toBe(true) + for (const id of ["2", "e1", "3"]) { + expect(result.fixed).toContain(` { + const xml = `` + const result = validateAndFixXml(xml) + expect(result.valid).toBe(true) + expect(result.fixed).toContain(' { + const xml = `` + + it("deletes a wrapped cell and its edges", () => { + const { result, errors } = applyDiagramOperations(xml, [ + { operation: "delete", cell_id: "5" }, + { operation: "delete", cell_id: "e1" }, + ]) + expect(errors).toEqual([]) + expect(result).not.toContain("UserObject") + expect(result).not.toContain('id="e1"') + expect(result).toContain('id="6"') + }) + + it("rejects adding a cell with the id of a wrapped cell", () => { + const { errors } = applyDiagramOperations(xml, [ + { + operation: "add", + cell_id: "5", + new_xml: '', + }, + ]) + expect(errors[0]?.message).toContain("already exists") + }) + + it("updates a wrapped cell", () => { + const { result, errors } = applyDiagramOperations(xml, [ + { + operation: "update", + cell_id: "5", + new_xml: + '', + }, + ]) + expect(errors).toEqual([]) + expect(result).toContain('label="New"') + expect(result).not.toContain('label="Docs"') + }) + + it("reports deleting a cell that does not exist", () => { + const { errors } = applyDiagramOperations(xml, [ + { operation: "delete", cell_id: "missing" }, + ]) + expect(errors[0]?.message).toContain("not found") + }) +})