Address PR review: fix edge regex, forward refs, add Cloud/layout/parser tests

Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>

Author: Copilot

src/DiagramForge/Parsers/Mermaid/MermaidArchitectureParser.cs

@@ -28,7 +28,7 @@ internal sealed partial class MermaidArchitectureParser : IMermaidDiagramParser
     // The {group} suffix is optional and stripped from the node ID.
     // Arrow may be --> (directed) or -- (undirected).
     [GeneratedRegex(
-        @"^(?<src>\w+)(?:\{group\})?:(?<srcPort>[LRTB])\s*(?<arrow>--?>?)\s*(?<dstPort>[LRTB]):(?<dst>\w+)(?:\{group\})?$",
+        @"^(?<src>\w+)(?:\{group\})?:(?<srcPort>[LRTB])\s*(?<arrow>-->|--)\s*(?<dstPort>[LRTB]):(?<dst>\w+)(?:\{group\})?$",
         RegexOptions.CultureInvariant)]
     private static partial Regex EdgePattern();
 
@@ -42,28 +42,42 @@ public Diagram Parse(MermaidDocument document)
 
         var diagram = builder.Build();
 
-        var groups = new Dictionary<string, DiagramGroup>(StringComparer.Ordinal);
-
+        // ── Pass 1: register all groups and collect parent relationships ──────────
+        // Pre-registering every group ID allows forward references in `in parentId`
+        // declarations to resolve correctly in pass 2.
+        var groupParents = new Dictionary<string, string>(StringComparer.Ordinal);
         for (int i = 1; i < document.Lines.Length; i++)
         {
-            var line = document.Lines[i];
+            var groupMatch = GroupPattern().Match(document.Lines[i]);
+            if (!groupMatch.Success)
+                continue;
 
-            var groupMatch = GroupPattern().Match(line);
-            if (groupMatch.Success)
-            {
-                var id = groupMatch.Groups["id"].Value;
-                var label = groupMatch.Groups["label"].Value;
-                var parent = groupMatch.Groups["parent"].Value;
+            var id = groupMatch.Groups["id"].Value;
+            var label = groupMatch.Groups["label"].Value;
+            var parent = groupMatch.Groups["parent"].Value;
 
-                var group = new DiagramGroup(id, label);
-                groups[id] = group;
-                diagram.AddGroup(group);
+            diagram.AddGroup(new DiagramGroup(id, label));
+            if (!string.IsNullOrEmpty(parent))
+                groupParents[id] = parent;
+        }
 
-                if (!string.IsNullOrEmpty(parent) && groups.TryGetValue(parent, out var parentGroup))
-                    parentGroup.ChildGroupIds.Add(id);
+        var groups = diagram.Groups.ToDictionary(g => g.Id, StringComparer.Ordinal);
+
+        // Apply group nesting now that all group IDs are registered.
+        foreach (var (childId, parentId) in groupParents)
+        {
+            if (groups.TryGetValue(parentId, out var parentGroup))
+                parentGroup.ChildGroupIds.Add(childId);
+        }
 
+        // ── Pass 2: services, junctions, and edges ────────────────────────────────
+        for (int i = 1; i < document.Lines.Length; i++)
+        {
+            var line = document.Lines[i];
+
+            // Skip group lines — already handled in pass 1.
+            if (GroupPattern().IsMatch(line))
                 continue;
-            }
 
             var serviceMatch = ServicePattern().Match(line);
             if (serviceMatch.Success)
@@ -106,7 +120,7 @@ public Diagram Parse(MermaidDocument document)
 
                 var edge = new Edge(srcId, dstId)
                 {
-                    ArrowHead = arrow.Contains('>') ? ArrowHeadStyle.Arrow : ArrowHeadStyle.None,
+                    ArrowHead = arrow == "-->" ? ArrowHeadStyle.Arrow : ArrowHeadStyle.None,
                     LineStyle = EdgeLineStyle.Solid,
                 };
                 edge.Metadata["source:port"] = srcPort;

tests/DiagramForge.Tests/Layout/DefaultLayoutEngineTests.cs

@@ -327,4 +327,123 @@ public void Layout_BlockDiagram_SpaceGapMovesNodeToLaterColumn()
         Assert.True(right.X > left.X + left.Width + diagram.LayoutHints.HorizontalSpacing,
             $"Expected right node X {right.X} to reflect a skipped middle column after left node right edge {left.X + left.Width}");
     }
+
+    // ── Architecture diagram layout ───────────────────────────────────────────
+
+    private static Diagram BuildArchitectureDiagram(Action<Diagram> configure)
+    {
+        var diagram = new Diagram { DiagramType = "architecture" };
+        configure(diagram);
+        return diagram;
+    }
+
+    private static Edge ArchEdge(string srcId, string srcPort, string dstPort, string dstId, bool directed = false)
+    {
+        var edge = new Edge(srcId, dstId)
+        {
+            ArrowHead = directed ? ArrowHeadStyle.Arrow : ArrowHeadStyle.None,
+        };
+        edge.Metadata["source:port"] = srcPort;
+        edge.Metadata["target:port"] = dstPort;
+        return edge;
+    }
+
+    [Fact]
+    public void Layout_Architecture_LREdge_PlacesSourceLeftOfTarget()
+    {
+        // db:R -- L:server → db is left of server (same row)
+        var diagram = BuildArchitectureDiagram(d =>
+            d.AddNode(new Node("db", "Database"))
+             .AddNode(new Node("server", "Server"))
+             .AddEdge(ArchEdge("db", "R", "L", "server")));
+
+        _engine.Layout(diagram, _theme);
+
+        var db = diagram.Nodes["db"];
+        var srv = diagram.Nodes["server"];
+
+        Assert.True(db.X < srv.X, $"db.X ({db.X}) should be left of server.X ({srv.X}) for R--L edge");
+        // Same row: Y coordinates should be equal
+        Assert.Equal(db.Y, srv.Y);
+    }
+
+    [Fact]
+    public void Layout_Architecture_TBEdge_PlacesSourceAboveTarget()
+    {
+        // disk:B -- T:server → disk is above server (same column)
+        var diagram = BuildArchitectureDiagram(d =>
+            d.AddNode(new Node("disk", "Storage"))
+             .AddNode(new Node("server", "Server"))
+             .AddEdge(ArchEdge("disk", "B", "T", "server")));
+
+        _engine.Layout(diagram, _theme);
+
+        var disk = diagram.Nodes["disk"];
+        var srv = diagram.Nodes["server"];
+
+        Assert.True(disk.Y < srv.Y, $"disk.Y ({disk.Y}) should be above server.Y ({srv.Y}) for B--T edge");
+        // Same column: X coordinates should be equal
+        Assert.Equal(disk.X, srv.X);
+    }
+
+    [Fact]
+    public void Layout_Architecture_DisconnectedNode_PlacedInSeparateRow()
+    {
+        // 'loner' has no port edges; it should end up in a different row from the connected pair
+        var diagram = BuildArchitectureDiagram(d =>
+            d.AddNode(new Node("a", "A"))
+             .AddNode(new Node("b", "B"))
+             .AddNode(new Node("loner", "Loner"))
+             .AddEdge(ArchEdge("a", "R", "L", "b")));
+
+        _engine.Layout(diagram, _theme);
+
+        var a = diagram.Nodes["a"];
+        var loner = diagram.Nodes["loner"];
+
+        // The disconnected node must be placed in a distinct row (different Y)
+        Assert.NotEqual(a.Y, loner.Y);
+    }
+
+    [Fact]
+    public void Layout_Architecture_AllPositionsNonNegative()
+    {
+        var diagram = BuildArchitectureDiagram(d =>
+            d.AddNode(new Node("db", "Database"))
+             .AddNode(new Node("server", "Server"))
+             .AddEdge(ArchEdge("db", "R", "L", "server")));
+
+        _engine.Layout(diagram, _theme);
+
+        Assert.All(diagram.Nodes.Values, n =>
+        {
+            Assert.True(n.X >= 0, $"{n.Id}.X = {n.X}");
+            Assert.True(n.Y >= 0, $"{n.Id}.Y = {n.Y}");
+        });
+    }
+
+    [Fact]
+    public void Layout_Architecture_WithGroup_GroupEnclosesMembers()
+    {
+        var diagram = BuildArchitectureDiagram(d =>
+        {
+            d.AddNode(new Node("db", "Database"));
+            d.AddNode(new Node("server", "Server"));
+            d.AddEdge(ArchEdge("db", "R", "L", "server"));
+            var group = new Group("api", "API");
+            group.ChildNodeIds.AddRange(["db", "server"]);
+            d.AddGroup(group);
+        });
+
+        _engine.Layout(diagram, _theme);
+
+        var group = diagram.Groups[0];
+        foreach (var n in diagram.Nodes.Values)
+        {
+            Assert.True(group.X <= n.X, $"group.X ({group.X}) should be <= {n.Id}.X ({n.X})");
+            Assert.True(group.Y <= n.Y, $"group.Y ({group.Y}) should be <= {n.Id}.Y ({n.Y})");
+            Assert.True(group.X + group.Width >= n.X + n.Width, "group right edge should cover node right edge");
+            Assert.True(group.Y + group.Height >= n.Y + n.Height, "group bottom edge should cover node bottom edge");
+        }
+    }
 }

tests/DiagramForge.Tests/Parsers/Mermaid/MermaidArchitectureParserTests.cs

@@ -216,6 +216,57 @@ service subnet(internet)[Subnet]
         Assert.Equal(ArrowHeadStyle.Arrow, edge.ArrowHead);
     }
 
+    [Fact]
+    public void Parse_InvalidArrowSyntax_IsNotTreatedAsEdge()
+    {
+        // "->"-style (single dash + >) is not valid architecture-beta edge syntax.
+        // The parser should not create an edge for it.
+        const string text = """
+            architecture-beta
+              service db(database)[Database]
+              service server(server)[Server]
+              db:L -> R:server
+            """;
+
+        var diagram = _parser.Parse(text);
+
+        // No edges should be created from the malformed line
+        Assert.Empty(diagram.Edges);
+    }
+
+    [Fact]
+    public void Parse_ServiceBeforeGroup_ForwardRefResolvesCorrectly()
+    {
+        // Service declared before its parent group: forward reference must still resolve.
+        const string text = """
+            architecture-beta
+              service db(database)[Database] in api
+              group api(cloud)[API]
+            """;
+
+        var diagram = _parser.Parse(text);
+
+        var group = Assert.Single(diagram.Groups);
+        Assert.Contains("db", group.ChildNodeIds);
+    }
+
+    [Fact]
+    public void Parse_NestedGroupForwardRef_ResolvesCorrectly()
+    {
+        // Child group declared before parent group.
+        const string text = """
+            architecture-beta
+              group private_api(cloud)[Private API] in public_api
+              group public_api(cloud)[Public API]
+            """;
+
+        var diagram = _parser.Parse(text);
+
+        Assert.Equal(2, diagram.Groups.Count);
+        var parent = diagram.Groups.Single(g => g.Id == "public_api");
+        Assert.Contains("private_api", parent.ChildGroupIds);
+    }
+
     // ── Comments ──────────────────────────────────────────────────────────────
 
     [Fact]

tests/DiagramForge.Tests/Rendering/SvgRendererTests.cs

@@ -200,6 +200,21 @@ public void Render_NullTheme_ThrowsArgumentNullException()
             _renderer.Render(new Diagram(), null!));
     }
 
+    // ── Shape rendering ───────────────────────────────────────────────────────
+
+    [Fact]
+    public void Render_CloudShape_ProducesPathElement()
+    {
+        var diagram = BuildAndLayout(new Diagram()
+            .AddNode(new Node("A", "Cloud Service") { Shape = Shape.Cloud }));
+
+        string svg = _renderer.Render(diagram, _theme);
+
+        // Cloud is rendered as a <path d="..." />, not a <rect> or <polygon>.
+        Assert.Contains("<path d=", svg);
+        Assert.Contains("Cloud Service", svg);
+    }
+
     // ── Utilities (mirrors the production SvgRenderer.F() helper) ────────────
 
     private static string F(double v) =>