Address review feedback: quote node IDs, fix edge duplication, include MermaidJS in output

- Quote all node IDs with single quotes to handle reserved keywords (e.g., 'end')
- Include MermaidJS script directly in display_workflow_diagram output
- Use mark_safe() to prevent Django from escaping HTML
- Remove get_graph_mermaid() method (not needed)
- Fix edge duplication by tracking edges in a dict and only adding once
- Update edge styling to replace (not duplicate) gray default with active styling
- Update tests to match new quoted node ID format
- Remove tests for deleted get_graph_mermaid() method

Co-authored-by: codingjoe <[email protected]>

Author: Copilot

joeflow/admin.py

@@ -2,6 +2,7 @@
 from django.contrib.auth import get_permission_codename
 from django.db import transaction
 from django.utils.html import format_html
+from django.utils.safestring import mark_safe
 from django.utils.translation import gettext_lazy as t
 
 from . import forms, models
@@ -159,11 +160,17 @@ def display_workflow_diagram(self, obj):
         if obj.pk:
             # Get Mermaid diagram syntax
             mermaid_syntax = obj.get_instance_graph_mermaid()
-            # Wrap in div with mermaid class for client-side rendering
-            return format_html(
-                '<div class="mermaid-diagram"><div class="mermaid">{}</div></div>',
-                mermaid_syntax
-            )
+            # Include MermaidJS script and wrap diagram
+            html = f"""
+            <script type="module">
+              import mermaid from 'https://cdn.jsdelivr.net/npm/mermaid@10/dist/mermaid.esm.min.mjs';
+              mermaid.initialize({{ startOnLoad: true }});
+            </script>
+            <div class="mermaid-diagram">
+              <div class="mermaid">{mermaid_syntax}</div>
+            </div>
+            """
+            return mark_safe(html)
         return ""
 
     @transaction.atomic()

joeflow/models.py

@@ -242,40 +242,6 @@ def get_graph_svg(cls):
 
     get_graph_svg.short_description = t("graph")
 
-    @classmethod
-    def get_graph_mermaid(cls, color="black"):
-        """
-        Return workflow graph as Mermaid diagram syntax.
-
-        This can be used with MermaidJS for client-side rendering in browsers.
-
-        Returns:
-            (str): Mermaid diagram syntax.
-        """
-        lines = [f"graph {cls.rankdir}"]
-        
-        # Add nodes
-        for name, node in cls.get_nodes():
-            node_id = name.replace(" ", "_")
-            # Keep original name with spaces for label
-            label = name.replace("_", " ")
-            
-            # Determine shape based on node type
-            if node.type == HUMAN:
-                # Rounded rectangle for human tasks
-                lines.append(f"    {node_id}({label})")
-            else:
-                # Rectangle for machine tasks
-                lines.append(f"    {node_id}[{label}]")
-        
-        # Add edges
-        for start, end in cls.edges:
-            start_id = start.name.replace(" ", "_")
-            end_id = end.name.replace(" ", "_")
-            lines.append(f"    {start_id} --> {end_id}")
-        
-        return "\n".join(lines)
-
     def get_instance_graph(self):
         """Return workflow instance graph."""
         graph = self.get_graph(color="#888888")
@@ -377,8 +343,8 @@ def get_instance_graph_mermaid(self):
         """
         lines = [f"graph {self.rankdir}"]
         node_styles = []
-        edge_styles = []
-        edge_index = 0
+        edge_styles = {}  # Map of (start, end) -> style
+        edge_list = []  # List to maintain order of edges
 
         names = dict(self.get_nodes()).keys()
 
@@ -388,40 +354,43 @@ def get_instance_graph_mermaid(self):
             # Keep original name with spaces for label
             label = name.replace("_", " ")
 
-            # Determine shape based on node type
+            # Determine shape based on node type, quote IDs to handle reserved words
             if node.type == HUMAN:
-                lines.append(f"    {node_id}({label})")
+                lines.append(f"    '{node_id}'({label})")
             else:
-                lines.append(f"    {node_id}[{label}]")
+                lines.append(f"    '{node_id}'[{label}]")
 
             # Default gray styling for nodes not yet processed
-            node_styles.append(f"    style {node_id} fill:#f9f9f9,stroke:#999,color:#999")
+            node_styles.append(f"    style '{node_id}' fill:#f9f9f9,stroke:#999,color:#999")
 
-        # Add edges from workflow definition (gray style)
+        # Add edges from workflow definition with default gray style
         for start, end in self.edges:
             start_id = start.name.replace(" ", "_")
             end_id = end.name.replace(" ", "_")
-            lines.append(f"    {start_id} --> {end_id}")
-            edge_styles.append(f"    linkStyle {edge_index} stroke:#999")
-            edge_index += 1
+            edge_key = (start_id, end_id)
+            if edge_key not in edge_styles:
+                edge_list.append(edge_key)
+                edge_styles[edge_key] = "stroke:#999"
 
         # Process actual tasks to highlight active/completed states
         for task in self.task_set.filter(name__in=names):
             node_id = task.name.replace(" ", "_")
 
             # Active tasks (not completed) get bold black styling
             if not task.completed:
-                node_styles.append(f"    style {node_id} fill:#fff,stroke:#000,stroke-width:3px,color:#000")
+                node_styles.append(f"    style '{node_id}' fill:#fff,stroke:#000,stroke-width:3px,color:#000")
             else:
                 # Completed tasks get normal black styling
-                node_styles.append(f"    style {node_id} fill:#fff,stroke:#000,stroke-width:2px,color:#000")
+                node_styles.append(f"    style '{node_id}' fill:#fff,stroke:#000,stroke-width:2px,color:#000")
 
-            # Add edges for actual task connections (black style)
+            # Update edge styling for actual task connections (black style)
             for child in task.child_task_set.exclude(name="override"):
                 child_id = child.name.replace(" ", "_")
-                lines.append(f"    {node_id} --> {child_id}")
-                edge_styles.append(f"    linkStyle {edge_index} stroke:#000,stroke-width:2px")
-                edge_index += 1
+                edge_key = (node_id, child_id)
+                if edge_key not in edge_styles:
+                    edge_list.append(edge_key)
+                # Update styling to black (overrides gray)
+                edge_styles[edge_key] = "stroke:#000,stroke-width:2px"
 
         # Handle override tasks
         for task in self.task_set.filter(name="override").prefetch_related(
@@ -430,57 +399,75 @@ def get_instance_graph_mermaid(self):
             override_id = f"override_{task.pk}"
             override_label = f"override {task.pk}"
 
-            # Add override node with dashed style
-            lines.append(f"    {override_id}({override_label})")
-            node_styles.append(f"    style {override_id} fill:#fff,stroke:#000,stroke-width:2px,stroke-dasharray:5 5,color:#000")
+            # Add override node with dashed style, quote ID
+            lines.append(f"    '{override_id}'({override_label})")
+            node_styles.append(f"    style '{override_id}' fill:#fff,stroke:#000,stroke-width:2px,stroke-dasharray:5 5,color:#000")
 
             # Add dashed edges for override connections
             for parent in task.parent_task_set.all():
                 parent_id = parent.name.replace(" ", "_")
-                lines.append(f"    {parent_id} -.-> {override_id}")
-                edge_styles.append(f"    linkStyle {edge_index} stroke:#000,stroke-dasharray:5 5")
-                edge_index += 1
+                edge_key = (parent_id, override_id)
+                if edge_key not in edge_styles:
+                    edge_list.append(edge_key)
+                edge_styles[edge_key] = "stroke:#000,stroke-dasharray:5 5"
 
             for child in task.child_task_set.all():
                 child_id = child.name.replace(" ", "_")
-                lines.append(f"    {override_id} -.-> {child_id}")
-                edge_styles.append(f"    linkStyle {edge_index} stroke:#000,stroke-dasharray:5 5")
-                edge_index += 1
+                edge_key = (override_id, child_id)
+                if edge_key not in edge_styles:
+                    edge_list.append(edge_key)
+                edge_styles[edge_key] = "stroke:#000,stroke-dasharray:5 5"
 
         # Handle obsolete/custom tasks (not in workflow definition)
         for task in self.task_set.exclude(name__in=names).exclude(name="override"):
             node_id = task.name.replace(" ", "_")
             # Keep original name with spaces for label
             label = task.name.replace("_", " ")
 
-            # Determine shape based on node type
+            # Determine shape based on node type, quote IDs
             if task.type == HUMAN:
-                lines.append(f"    {node_id}({label})")
+                lines.append(f"    '{node_id}'({label})")
             else:
-                lines.append(f"    {node_id}[{label}]")
+                lines.append(f"    '{node_id}'[{label}]")
 
             # Dashed styling for obsolete tasks
             if not task.completed:
-                node_styles.append(f"    style {node_id} fill:#fff,stroke:#000,stroke-width:3px,stroke-dasharray:5 5,color:#000")
+                node_styles.append(f"    style '{node_id}' fill:#fff,stroke:#000,stroke-width:3px,stroke-dasharray:5 5,color:#000")
             else:
-                node_styles.append(f"    style {node_id} fill:#fff,stroke:#000,stroke-width:2px,stroke-dasharray:5 5,color:#000")
+                node_styles.append(f"    style '{node_id}' fill:#fff,stroke:#000,stroke-width:2px,stroke-dasharray:5 5,color:#000")
 
             # Add dashed edges for obsolete task connections
             for parent in task.parent_task_set.all():
                 parent_id = parent.name.replace(" ", "_")
-                lines.append(f"    {parent_id} -.-> {node_id}")
-                edge_styles.append(f"    linkStyle {edge_index} stroke:#000,stroke-dasharray:5 5")
-                edge_index += 1
+                edge_key = (parent_id, node_id)
+                if edge_key not in edge_styles:
+                    edge_list.append(edge_key)
+                edge_styles[edge_key] = "stroke:#000,stroke-dasharray:5 5"
 
             for child in task.child_task_set.all():
                 child_id = child.name.replace(" ", "_")
-                lines.append(f"    {node_id} -.-> {child_id}")
-                edge_styles.append(f"    linkStyle {edge_index} stroke:#000,stroke-dasharray:5 5")
-                edge_index += 1
+                edge_key = (node_id, child_id)
+                if edge_key not in edge_styles:
+                    edge_list.append(edge_key)
+                edge_styles[edge_key] = "stroke:#000,stroke-dasharray:5 5"
+        
+        # Add edges to output (using dotted arrow for dashed edges)
+        for start_id, end_id in edge_list:
+            style = edge_styles[(start_id, end_id)]
+            if "dasharray" in style:
+                # Use dotted arrow for dashed edges
+                lines.append(f"    '{start_id}' -.-> '{end_id}'")
+            else:
+                # Use solid arrow for normal edges
+                lines.append(f"    '{start_id}' --> '{end_id}'")
 
         # Add all styling at the end
         lines.extend(node_styles)
-        lines.extend(edge_styles)
+        
+        # Add edge styling
+        for idx, (start_id, end_id) in enumerate(edge_list):
+            style = edge_styles[(start_id, end_id)]
+            lines.append(f"    linkStyle {idx} {style}")
 
         return "\n".join(lines)
 

tests/test_models.py

@@ -174,34 +174,6 @@ def test_get_instance_graph_svg(self, db, fixturedir):
         svg = wf.get_instance_graph_svg()
         assert isinstance(svg, SafeString)
 
-    def test_get_graph_mermaid(self):
-        """Test that get_graph_mermaid returns valid Mermaid syntax."""
-        mermaid = workflows.SimpleWorkflow.get_graph_mermaid()
-        
-        # Check it's a string
-        assert isinstance(mermaid, str)
-        
-        # Check it starts with graph declaration
-        assert mermaid.startswith("graph LR") or mermaid.startswith("graph TD")
-        
-        # Check it contains nodes
-        assert "start_method[start method]" in mermaid
-        assert "save_the_princess(save the princess)" in mermaid  # HUMAN task, rounded
-        assert "end[end]" in mermaid
-        
-        # Check it contains edges
-        assert "start_method --> save_the_princess" in mermaid
-        assert "save_the_princess --> end" in mermaid
-
-    def test_get_graph_mermaid_with_direction(self):
-        """Test that get_graph_mermaid respects rankdir."""
-        workflows.SimpleWorkflow.rankdir = "TD"
-        mermaid = workflows.SimpleWorkflow.get_graph_mermaid()
-        assert mermaid.startswith("graph TD")
-        
-        # Reset to default
-        workflows.SimpleWorkflow.rankdir = "LR"
-
     def test_get_instance_graph_mermaid(self, db):
         """Test that get_instance_graph_mermaid returns valid Mermaid syntax with task states."""
         wf = workflows.SimpleWorkflow.start_method()
@@ -213,12 +185,12 @@ def test_get_instance_graph_mermaid(self, db):
         # Check it starts with graph declaration
         assert mermaid.startswith("graph LR") or mermaid.startswith("graph TD")
 
-        # Check it contains nodes
-        assert "save_the_princess(save the princess)" in mermaid
-        assert "start_method[start method]" in mermaid
+        # Check it contains nodes with quoted IDs
+        assert "'save_the_princess'(save the princess)" in mermaid
+        assert "'start_method'[start method]" in mermaid
 
-        # Check it contains edges
-        assert "start_method --> save_the_princess" in mermaid
+        # Check it contains edges with quoted IDs
+        assert "'start_method' --> 'save_the_princess'" in mermaid
 
         # Check it contains styling (for active/completed tasks)
         assert "style " in mermaid
@@ -259,14 +231,14 @@ def test_get_instance_graph_mermaid_with_obsolete(self, db):
 
         mermaid = workflow.get_instance_graph_mermaid()
 
-        # Check obsolete node exists
-        assert "obsolete[obsolete]" in mermaid
+        # Check obsolete node exists with quoted ID
+        assert "'obsolete'[obsolete]" in mermaid
 
         # Check dashed edges (dotted arrow notation in Mermaid)
-        assert "-.->obsolete" in mermaid.replace(" ", "") or "obsolete-.->end" in mermaid.replace(" ", "")
+        assert "'start_method' -.-> 'obsolete'" in mermaid or "'obsolete' -.-> 'end'" in mermaid
 
         # Check obsolete task styling with dashed border
-        assert "style obsolete" in mermaid
+        assert "style 'obsolete'" in mermaid
         assert "stroke-dasharray" in mermaid
 
     def test_cancel(self, db):