Refactor Mermaid parser into facade and flowchart subparser#19
Conversation
Split Mermaid parsing by diagram type behind a single MermaidParser facade. Add MermaidDocument normalization and kind detection, an internal dispatch interface, and a flowchart-specific parser containing the existing behavior. Add parser-selection tests covering leading comments/whitespace dispatch and unsupported Mermaid kinds. Mermaid parser tests and E2E snapshots remain green.
There was a problem hiding this comment.
Pull request overview
This PR refactors the monolithic MermaidParser into a facade + subparser pattern. MermaidParser now acts as a dispatcher backed by an IMermaidDiagramParser internal interface, with the existing flowchart parsing logic extracted into MermaidFlowchartParser. A new MermaidDocument class handles input normalization (stripping comments/whitespace, trimming lines) and diagram kind detection. Tests are added for leading-comment dispatch and unsupported diagram type handling.
Changes:
- Extract flowchart parsing logic into
MermaidFlowchartParser(newIMermaidDiagramParserimplementation) and reduceMermaidParserto a dispatch facade. - Add
MermaidDocument(normalization + kind detection) andMermaidDiagramKindenum +IMermaidDiagramParserinterface as the dispatch layer. - Add tests for leading-comment dispatch and unsupported Mermaid diagram type error paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/DiagramForge/Parsers/Mermaid/MermaidParser.cs |
Reduced to a facade that delegates to IMermaidDiagramParser sub-parsers via MermaidDocument |
src/DiagramForge/Parsers/Mermaid/MermaidFlowchartParser.cs |
New file containing the flowchart parsing logic extracted from the old MermaidParser |
src/DiagramForge/Parsers/Mermaid/MermaidDocument.cs |
New file for input normalization, comment stripping, and diagram kind detection |
src/DiagramForge/Parsers/Mermaid/MermaidDiagramKind.cs |
New enum for identified Mermaid diagram types |
src/DiagramForge/Parsers/Mermaid/IMermaidDiagramParser.cs |
New internal dispatch interface for type-specific parsers |
tests/DiagramForge.Tests/Parsers/MermaidParserTests.cs |
New tests for leading-comment dispatch and unsupported diagram type handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (parser is null) | ||
| { | ||
| var (group, members) = groupStack.Pop(); | ||
| // Sort for determinism — HashSet enumeration order is undefined and | ||
| // group membership flows into SVG output via layout. | ||
| group.ChildNodeIds.AddRange(members.OrderBy(m => m, StringComparer.Ordinal)); | ||
| builder.AddGroup(group); | ||
| throw new DiagramParseException( | ||
| $"Mermaid diagram type '{document.Kind}' is not supported. " + | ||
| $"Supported Mermaid types: {string.Join(", ", SupportedDiagramTypes)}"); | ||
| } |
There was a problem hiding this comment.
The null-parser guard at lines 37–42 is unreachable with the current design. MermaidDocument.Parse (line 35) calls MermaidDocument.TryParse internally, which only returns a document when TryDetectKind succeeds — and TryDetectKind currently only recognizes MermaidDiagramKind.Flowchart, for which MermaidFlowchartParser.CanParse always returns true. As a result, MermaidDocument.Parse throws its generic error for every unrecognized type before execution ever reaches the null-parser check, making lines 37–42 dead code.
This also means the more informative error message here (which includes the concrete kind name and the supported-types list) will never surface to callers. If the intent is to support future kinds that are recognized by TryDetectKind but have no registered IMermaidDiagramParser yet, consider redesigning: for example, have TryDetectKind accept all known Mermaid keyword headers (returning an Unknown kind for unrecognized ones) so that both failure modes flow through MermaidParser.Parse where context-appropriate messages can be emitted.
| if (TryParse(diagramText, out var document) && document is not null) | ||
| return document; | ||
|
|
||
| throw new DiagramParseException("Diagram text cannot be null, empty, or an unsupported Mermaid diagram type."); |
There was a problem hiding this comment.
The MermaidDocument.Parse error message at line 48 is the same for both "empty/whitespace input" and "unrecognized diagram type" failure modes. When a user submits "sequenceDiagram\n...", the message doesn't identify which diagram type was encountered. The first line of the input (the diagram type token) is available at the call site in MermaidParser.Parse and could be included to give actionable feedback, for example: "Unsupported Mermaid diagram type 'sequenceDiagram'.".
Additionally, the document is not null guard in the && condition is redundant: when TryParse returns true it always assigns a non-null value to document, so the compiler's flow analysis already knows document is non-null in the success branch. The guard can be removed without changing behavior, but its presence may indicate uncertainty about the contract.
| if (TryParse(diagramText, out var document) && document is not null) | |
| return document; | |
| throw new DiagramParseException("Diagram text cannot be null, empty, or an unsupported Mermaid diagram type."); | |
| if (TryParse(diagramText, out var document)) | |
| return document; | |
| if (string.IsNullOrWhiteSpace(diagramText)) | |
| throw new DiagramParseException("Diagram text cannot be null or whitespace."); | |
| var firstContentLine = diagramText | |
| .Split('\n') | |
| .Select(line => line.Trim()) | |
| .FirstOrDefault(line => !string.IsNullOrEmpty(line) && !line.StartsWith("%%", StringComparison.Ordinal)); | |
| if (firstContentLine is null) | |
| throw new DiagramParseException("Diagram text cannot be empty or contain only comments."); | |
| throw new DiagramParseException($"Unsupported Mermaid diagram type '{firstContentLine}'."); |
|
@copilot address comments |
… parser - Add MermaidDiagramKind.Unknown for recognized-but-unsupported Mermaid types - Update TryDetectKind to return Unknown for known Mermaid keywords (sequenceDiagram, classDiagram, gantt, etc.) so they bypass the generic error and reach MermaidParser - Fix dead null-parser guard in MermaidParser.Parse: now reachable for Unknown kind, emitting an informative message with the concrete type token - Update MermaidParser.CanParse to check registered parsers (returns false for Unknown) - Differentiate MermaidDocument.Parse error messages: whitespace vs. comment-only vs. unrecognized type - Remove redundant `document is not null` guard; add [NotNullWhen(true)] to TryParse - Use IndexOf/slice instead of Split for first-token extraction (avoids array allocation) Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Addressed both review comments in 066dc47: MermaidParser.cs null-parser guard (now reachable): MermaidDocument.Parse error messages and redundant guard: Replaced the single generic message with three differentiated errors — null/whitespace input, comment-only input, and unrecognized diagram type (including the first content line in the message). Removed the redundant |
Split Mermaid parsing by diagram type behind a single MermaidParser facade. Add MermaidDocument normalization and kind detection, an internal dispatch interface, and a flowchart-specific parser containing the existing behavior.
Add parser-selection tests covering leading comments/whitespace dispatch and unsupported Mermaid kinds. Mermaid parser tests and E2E snapshots remain green.