-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve Mermaid diagram error handling with helpful suggestions #6715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,6 +95,53 @@ export default function MermaidBlock({ code }: MermaidBlockProps) { | |
| const { showCopyFeedback, copyWithFeedback } = useCopyToClipboard() | ||
| const { t } = useAppTranslation() | ||
|
|
||
| // Helper function to enhance error messages with suggestions | ||
| const enhanceErrorMessage = (originalError: string, code: string): string => { | ||
| let enhancedMessage = originalError | ||
|
|
||
| // Check for common syntax errors | ||
| if (originalError.includes("LINK_ID") && originalError.includes("Expecting")) { | ||
| // Count brackets to check for unclosed brackets | ||
| const openBrackets = (code.match(/\[/g) || []).length | ||
| const closeBrackets = (code.match(/\]/g) || []).length | ||
| const openBraces = (code.match(/\{/g) || []).length | ||
| const closeBraces = (code.match(/\}/g) || []).length | ||
|
|
||
| if (openBrackets > closeBrackets) { | ||
| enhancedMessage += | ||
| "\n\nSuggestion: You have unclosed square brackets '['. Make sure all node labels are properly closed with ']'." | ||
| } | ||
| if (openBraces > closeBraces) { | ||
| enhancedMessage += | ||
| "\n\nSuggestion: You have unclosed curly braces '{'. Make sure all decision nodes are properly closed with '}'." | ||
| } | ||
|
|
||
| // Check for incomplete node definitions | ||
| if (code.includes("@") && !code.includes("]") && !code.includes("}")) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently only checking for '@' character, but could there be other special characters that might need proper enclosure in node labels? Perhaps we could expand this check to include other common special characters that cause parsing issues? |
||
| enhancedMessage += | ||
| "\n\nSuggestion: Node labels containing special characters like '@' should be properly enclosed in brackets." | ||
| } | ||
| } | ||
|
|
||
| // Check for other common issues | ||
| if (originalError.includes("Parse error") && code.trim().endsWith("-->")) { | ||
| enhancedMessage += | ||
| "\n\nSuggestion: Your diagram appears to end with an arrow '-->'. Make sure to complete the connection with a target node." | ||
| } | ||
|
|
||
| if ( | ||
| !code.trim().startsWith("graph") && | ||
| !code.trim().startsWith("flowchart") && | ||
| !code.trim().startsWith("sequenceDiagram") && | ||
| !code.trim().startsWith("classDiagram") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this list of diagram types comprehensive? Mermaid supports additional diagram types like |
||
| ) { | ||
| enhancedMessage += | ||
| "\n\nSuggestion: Make sure your diagram starts with a valid diagram type (e.g., 'flowchart TD', 'graph LR', 'sequenceDiagram', etc.)." | ||
| } | ||
|
|
||
| return enhancedMessage | ||
| } | ||
|
|
||
| // 1) Whenever `code` changes, mark that we need to re-render a new chart | ||
| useEffect(() => { | ||
| setIsLoading(true) | ||
|
|
@@ -121,7 +168,8 @@ export default function MermaidBlock({ code }: MermaidBlockProps) { | |
| }) | ||
| .catch((err) => { | ||
| console.warn("Mermaid parse/render failed:", err) | ||
| setError(err.message || "Failed to render Mermaid diagram") | ||
| const enhancedError = enhanceErrorMessage(err.message || "Failed to render Mermaid diagram", code) | ||
| setError(enhancedError) | ||
| }) | ||
| .finally(() => { | ||
| setIsLoading(false) | ||
|
|
@@ -207,7 +255,12 @@ export default function MermaidBlock({ code }: MermaidBlockProps) { | |
| backgroundColor: "var(--vscode-editor-background)", | ||
| borderTop: "none", | ||
| }}> | ||
| <div style={{ marginBottom: "8px", color: "var(--vscode-descriptionForeground)" }}> | ||
| <div | ||
| style={{ | ||
| marginBottom: "8px", | ||
| color: "var(--vscode-descriptionForeground)", | ||
| whiteSpace: "pre-wrap", | ||
| }}> | ||
| {error} | ||
| </div> | ||
| <CodeBlock language="mermaid" source={code} /> | ||
|
|
@@ -216,7 +269,11 @@ export default function MermaidBlock({ code }: MermaidBlockProps) { | |
| </div> | ||
| ) : ( | ||
| <MermaidButton containerRef={containerRef} code={code} isLoading={isLoading} svgToPng={svgToPng}> | ||
| <SvgContainer onClick={handleClick} ref={containerRef} $isLoading={isLoading}></SvgContainer> | ||
| <SvgContainer | ||
| onClick={handleClick} | ||
| ref={containerRef} | ||
| $isLoading={isLoading} | ||
| data-testid="svg-container"></SvgContainer> | ||
| </MermaidButton> | ||
| )} | ||
| </MermaidBlockContainer> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,287 @@ | ||
| import { render, screen, waitFor } from "@testing-library/react" | ||
| import userEvent from "@testing-library/user-event" | ||
| import { vi } from "vitest" | ||
| import mermaid from "mermaid" | ||
| import MermaidBlock from "../MermaidBlock" | ||
|
|
||
| // Mock mermaid module | ||
| vi.mock("mermaid", () => ({ | ||
| default: { | ||
| initialize: vi.fn(), | ||
| parse: vi.fn(), | ||
| render: vi.fn(), | ||
| }, | ||
| })) | ||
|
|
||
| // Mock vscode API | ||
| vi.mock("@src/utils/vscode", () => ({ | ||
| vscode: { | ||
| postMessage: vi.fn(), | ||
| }, | ||
| })) | ||
|
|
||
| // Mock translation hook | ||
| vi.mock("@src/i18n/TranslationContext", () => ({ | ||
| useAppTranslation: () => ({ | ||
| t: (key: string) => { | ||
| const translations: Record<string, string> = { | ||
| "common:mermaid.loading": "Loading diagram...", | ||
| "common:mermaid.render_error": "Failed to render diagram", | ||
| } | ||
| return translations[key] || key | ||
| }, | ||
| }), | ||
| })) | ||
|
|
||
| // Mock clipboard hook | ||
| let mockCopyWithFeedback = vi.fn() | ||
| vi.mock("@src/utils/clipboard", () => ({ | ||
| useCopyToClipboard: () => ({ | ||
| showCopyFeedback: false, | ||
| copyWithFeedback: mockCopyWithFeedback, | ||
| }), | ||
| })) | ||
|
|
||
| // Mock CodeBlock component | ||
| vi.mock("../CodeBlock", () => ({ | ||
| default: ({ source, language }: { source: string; language: string }) => ( | ||
| <div data-testid="code-block" data-language={language}> | ||
| {source} | ||
| </div> | ||
| ), | ||
| })) | ||
|
|
||
| // Mock MermaidButton component | ||
| vi.mock("@/components/common/MermaidButton", () => ({ | ||
| MermaidButton: ({ children }: { children: React.ReactNode }) => <div>{children}</div>, | ||
| })) | ||
|
|
||
| // Mock canvas API for SVG to PNG conversion | ||
| const mockToDataURL = vi.fn(() => "data:image/png;base64,mockpngdata") | ||
| const mockGetContext = vi.fn(() => ({ | ||
| fillStyle: "", | ||
| fillRect: vi.fn(), | ||
| drawImage: vi.fn(), | ||
| imageSmoothingEnabled: true, | ||
| imageSmoothingQuality: "high", | ||
| })) | ||
|
|
||
| HTMLCanvasElement.prototype.toDataURL = mockToDataURL | ||
| HTMLCanvasElement.prototype.getContext = mockGetContext as any | ||
|
|
||
| describe("MermaidBlock", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| mockCopyWithFeedback = vi.fn() | ||
| mockToDataURL.mockClear() | ||
| mockGetContext.mockClear() | ||
| }) | ||
|
|
||
| it("renders loading state initially", () => { | ||
| vi.mocked(mermaid.parse).mockReturnValue(new Promise(() => {})) // Never resolves | ||
| render(<MermaidBlock code="flowchart TD\n A --> B" />) | ||
| expect(screen.getByText("Loading diagram...")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("renders mermaid diagram successfully", async () => { | ||
| const svgContent = "<svg><text>Test Diagram</text></svg>" | ||
| vi.mocked(mermaid.parse).mockResolvedValue({} as any) | ||
| vi.mocked(mermaid.render).mockResolvedValue({ svg: svgContent } as any) | ||
|
|
||
| render(<MermaidBlock code="flowchart TD\n A --> B" />) | ||
|
|
||
| await waitFor(() => { | ||
| const container = screen.getByTestId("svg-container") | ||
| expect(container.innerHTML).toBe(svgContent) | ||
| }) | ||
| }) | ||
|
|
||
| describe("Error handling", () => { | ||
| it("displays error message when mermaid parsing fails", async () => { | ||
| const errorMessage = "Parse error on line 2: Expecting 'AMP', 'COLON', got 'LINK_ID'" | ||
| vi.mocked(mermaid.parse).mockRejectedValue(new Error(errorMessage)) | ||
|
|
||
| render(<MermaidBlock code="flowchart TD A[Users Credentials] --> B{AuthController@che" />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Failed to render diagram")).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| it("shows enhanced error message for unclosed brackets", async () => { | ||
| const errorMessage = "Parse error on line 2: Expecting 'AMP', 'COLON', got 'LINK_ID'" | ||
| vi.mocked(mermaid.parse).mockRejectedValue(new Error(errorMessage)) | ||
|
|
||
| render(<MermaidBlock code="flowchart TD A[Users Credentials --> B" />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Failed to render diagram")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| // Click to expand error | ||
| const errorHeader = screen.getByText("Failed to render diagram").parentElement | ||
| await userEvent.click(errorHeader!) | ||
|
|
||
| await waitFor(() => { | ||
| const errorDetails = screen.getByText(/You have unclosed square brackets/) | ||
| expect(errorDetails).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| it("shows enhanced error message for unclosed braces", async () => { | ||
| const errorMessage = "Parse error on line 2: Expecting 'AMP', 'COLON', got 'LINK_ID'" | ||
| vi.mocked(mermaid.parse).mockRejectedValue(new Error(errorMessage)) | ||
|
|
||
| render(<MermaidBlock code="flowchart TD A{Decision --> B" />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Failed to render diagram")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| // Click to expand error | ||
| const errorHeader = screen.getByText("Failed to render diagram").parentElement | ||
| await userEvent.click(errorHeader!) | ||
|
|
||
| await waitFor(() => { | ||
| const errorDetails = screen.getByText(/You have unclosed curly braces/) | ||
| expect(errorDetails).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| it("shows suggestion for incomplete arrow connections", async () => { | ||
| const errorMessage = "Parse error at end of input" | ||
| vi.mocked(mermaid.parse).mockRejectedValue(new Error(errorMessage)) | ||
|
|
||
| render(<MermaidBlock code="flowchart TD\n A --> " />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Failed to render diagram")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| // Click to expand error | ||
| const errorHeader = screen.getByText("Failed to render diagram").parentElement | ||
| await userEvent.click(errorHeader!) | ||
|
|
||
| await waitFor(() => { | ||
| const errorDetails = screen.getByText(/Your diagram appears to end with an arrow/) | ||
| expect(errorDetails).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| it("shows suggestion for missing diagram type", async () => { | ||
| const errorMessage = "Parse error on line 1" | ||
| vi.mocked(mermaid.parse).mockRejectedValue(new Error(errorMessage)) | ||
|
|
||
| render(<MermaidBlock code="A --> B" />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Failed to render diagram")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| // Click to expand error | ||
| const errorHeader = screen.getByText("Failed to render diagram").parentElement | ||
| await userEvent.click(errorHeader!) | ||
|
|
||
| await waitFor(() => { | ||
| const errorDetails = screen.getByText(/Make sure your diagram starts with a valid diagram type/) | ||
| expect(errorDetails).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be helpful to add a test case for the special character detection with '@'? I noticed the implementation checks for this scenario but there's no corresponding test to verify it works correctly. |
||
|
|
||
| it("shows code block when error is expanded", async () => { | ||
| const code = "flowchart TD A[Incomplete" | ||
| const errorMessage = "Parse error" | ||
| vi.mocked(mermaid.parse).mockRejectedValue(new Error(errorMessage)) | ||
|
|
||
| render(<MermaidBlock code={code} />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Failed to render diagram")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| // Click to expand error | ||
| const errorHeader = screen.getByText("Failed to render diagram").parentElement | ||
| await userEvent.click(errorHeader!) | ||
|
|
||
| await waitFor(() => { | ||
| const codeBlock = screen.getByTestId("code-block") | ||
| expect(codeBlock).toBeInTheDocument() | ||
| expect(codeBlock).toHaveAttribute("data-language", "mermaid") | ||
| expect(codeBlock).toHaveTextContent(code) | ||
| }) | ||
| }) | ||
|
|
||
| it("allows copying error message and code", async () => { | ||
| const code = "flowchart TD A[Incomplete" | ||
| const errorMessage = "Parse error" | ||
| vi.mocked(mermaid.parse).mockRejectedValue(new Error(errorMessage)) | ||
|
|
||
| render(<MermaidBlock code={code} />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Failed to render diagram")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| // Find and click copy button | ||
| const copyButton = screen.getByRole("button") | ||
| await userEvent.click(copyButton) | ||
|
|
||
| expect(mockCopyWithFeedback).toHaveBeenCalledWith( | ||
| expect.stringContaining(`Error: ${errorMessage}`), | ||
| expect.any(Object), | ||
| ) | ||
| expect(mockCopyWithFeedback).toHaveBeenCalledWith(expect.stringContaining("```mermaid"), expect.any(Object)) | ||
| }) | ||
| }) | ||
|
|
||
| it("renders mermaid diagram and allows interaction", async () => { | ||
| const svgContent = '<svg width="100" height="100"><rect width="100" height="100"></rect></svg>' | ||
| vi.mocked(mermaid.parse).mockResolvedValue({} as any) | ||
| vi.mocked(mermaid.render).mockResolvedValue({ svg: svgContent } as any) | ||
|
|
||
| render(<MermaidBlock code="flowchart TD\n A --> B" />) | ||
|
|
||
| await waitFor(() => { | ||
| const container = screen.getByTestId("svg-container") | ||
| expect(container.innerHTML).toBe(svgContent) | ||
| }) | ||
|
|
||
| // Verify the SVG container is clickable | ||
| const svgContainer = screen.getByTestId("svg-container") | ||
| expect(svgContainer).toBeInTheDocument() | ||
| expect(svgContainer).toHaveStyle({ cursor: "pointer" }) | ||
| }) | ||
|
|
||
| it("debounces diagram rendering", async () => { | ||
| const { rerender } = render(<MermaidBlock code="flowchart TD\n A --> B" />) | ||
|
|
||
| // Initial parse call | ||
| expect(mermaid.parse).toHaveBeenCalledTimes(0) | ||
|
|
||
| // Wait for debounce | ||
| await waitFor( | ||
| () => { | ||
| expect(mermaid.parse).toHaveBeenCalledTimes(1) | ||
| }, | ||
| { timeout: 600 }, | ||
| ) | ||
|
|
||
| // Quick re-renders should not trigger immediate parse | ||
| rerender(<MermaidBlock code="flowchart TD\n A --> C" />) | ||
| rerender(<MermaidBlock code="flowchart TD\n A --> D" />) | ||
|
|
||
| // Should still be 1 call | ||
| expect(mermaid.parse).toHaveBeenCalledTimes(1) | ||
|
|
||
| // Wait for new debounce | ||
| await waitFor( | ||
| () => { | ||
| expect(mermaid.parse).toHaveBeenCalledTimes(2) | ||
| }, | ||
| { timeout: 600 }, | ||
| ) | ||
|
|
||
| // Should parse the latest code | ||
| expect(mermaid.parse).toHaveBeenLastCalledWith("flowchart TD\\n A --> D") | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error suggestion messages are hardcoded in English. Could we consider using the translation system with the
t()function to support internationalization? This would make the error messages accessible to users in other languages.