Refactor event rendering and improve graph data generation#117
Refactor event rendering and improve graph data generation#117transientlunatic wants to merge 1 commit intov0.7-previewfrom
Conversation
- Removed the inline review indicator HTML generation and replaced it with a more structured approach using JavaScript. - Enhanced the workflow graph visualization by integrating Mermaid and ELK for better representation of dependencies. - Simplified the rendering of analysis nodes and their associated data, ensuring better maintainability and readability. - Added error handling for graph data generation and modal data rendering. - Updated `pyproject.toml` to include package data for CLI static files.
There was a problem hiding this comment.
Pull request overview
This PR refactors HTML report workflow rendering by moving from server-side, per-node HTML graph generation to a data-driven Mermaid (+ ELK) visualization, and packages the required JS bundle for offline/air-gapped report viewing.
Changes:
- Replace the inline workflow graph DOM rendering with Mermaid graph data + a bundled Mermaid/ELK renderer.
- Generate per-analysis modal data as hidden DOM nodes, and wire Mermaid click handlers to open the existing modal.
- Package and copy the Mermaid/ELK bundle into the report output directory; add a small webpack build setup under
asimov/cli/js.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds package data entries so the CLI theme and static JS (bundle) ship with the wheel. |
asimov/event.py |
Emits Mermaid graph data + global node map, and emits hidden modal data containers per analysis node. |
asimov/cli/report.py |
Copies JS bundle into the report directory; replaces old graph CSS/JS with Mermaid rendering + filter-driven re-rendering. |
asimov/cli/js/webpack.config.js |
Introduces webpack config to produce a single Mermaid+ELK bundle. |
asimov/cli/js/src/index.js |
Initializes Mermaid with ELK renderer and exposes it globally for the report page. |
asimov/cli/js/package.json |
Adds build scripts and dependencies for bundling Mermaid + ELK. |
asimov/cli/js/package-lock.json |
Locks JS dependency tree for reproducible bundling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if hasattr(node, 'pipeline') and node.pipeline else '') | ||
| prefix = _REVIEW_PREFIX.get(review_status, '') | ||
| label = f'{prefix}{node.name}\\n{pipeline_name}'.replace('"', '#quot;') | ||
| is_subject = (getattr(node, 'category', '') == 'subject_analyses') |
There was a problem hiding this comment.
label = ... .replace('"', '#quot;') uses #quot;, which isn’t a valid HTML entity and also doesn’t properly escape quotes for the Mermaid definition that later wraps labels in "...". This can break rendering (and potentially enable diagram-syntax injection) if node.name/pipeline_name contain quotes or backslashes. Prefer escaping for Mermaid syntax (or avoid manual escaping and instead build the Mermaid definition using a safe encoder).
| mid = _mid(node.name) | ||
| data_id = f"analysis-data-{self.name}-{node.name}" | ||
| node_map[mid] = data_id | ||
| status = (node.status or 'unknown') if hasattr(node, 'status') else 'unknown' | ||
| review_status, _ = get_review_info(node) | ||
| pipeline_name = (node.pipeline.name | ||
| if hasattr(node, 'pipeline') and node.pipeline else '') | ||
| prefix = _REVIEW_PREFIX.get(review_status, '') | ||
| label = f'{prefix}{node.name}\\n{pipeline_name}'.replace('"', '#quot;') | ||
| is_subject = (getattr(node, 'category', '') == 'subject_analyses') | ||
| nodes_data.append({ | ||
| 'id': mid, | ||
| 'label': label, | ||
| 'status': status, | ||
| 'review': review_status, | ||
| 'isSubject': is_subject, | ||
| 'dataId': data_id, | ||
| }) |
There was a problem hiding this comment.
data_id = f"analysis-data-{self.name}-{node.name}" is used as an HTML id and later looked up via getElementById. If self.name or node.name contain spaces, quotes, or other problematic characters, the generated DOM id can be invalid or hard to reference reliably. Consider sanitizing these values for DOM IDs (and keep a separate human-readable label for display).
| try: | ||
| import networkx as nx | ||
| import os as _os | ||
| from asimov.event import status_map |
There was a problem hiding this comment.
from asimov.event import status_map is importing the current module from within itself. This is unnecessary (since status_map is already in module scope) and can be fragile if this code is ever invoked during module initialization. Prefer referencing status_map directly.
| from asimov.event import status_map |
| function buildMermaidDef(graphData, filters) { | ||
| var visibleNodes = graphData.nodes.filter(function(n) { | ||
| if (filters.hiddenStatuses.has(n.status)) return false; | ||
| if (filters.hiddenReviews.has(n.review)) return false; | ||
| if (filters.onlyStatus && n.status !== filters.onlyStatus) return false; | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
The review filter state (asimovActiveFilters.onlyReview) is set in initializeFilters(), but buildMermaidDef() does not apply it, so the Mermaid graphs won’t reflect the active review filter (only legacy .asimov-analysis elements will). Either apply onlyReview inside buildMermaidDef()’s node filtering logic or remove the review-only filtering UI for graphs to avoid inconsistent behavior.
| hasVisible = gd.nodes.some(function(n) { | ||
| if (asimovActiveFilters.hiddenStatuses.has(n.status)) return false; | ||
| if (asimovActiveFilters.hiddenReviews.has(n.review)) return false; | ||
| if (asimovActiveFilters.onlyStatus && n.status !== asimovActiveFilters.onlyStatus) return false; |
There was a problem hiding this comment.
checkEventVisibility() uses the graph data to determine whether an event should be collapsed, but it doesn’t account for the active review-only filter (asimovActiveFilters.onlyReview). If you add review-only filtering for Mermaid graphs, this function should use the same visibility predicate as buildMermaidDef() to avoid collapsing/expanding events incorrectly.
| if (asimovActiveFilters.onlyStatus && n.status !== asimovActiveFilters.onlyStatus) return false; | |
| if (asimovActiveFilters.onlyStatus && n.status !== asimovActiveFilters.onlyStatus) return false; | |
| if (asimovActiveFilters.onlyReview && n.review !== asimovActiveFilters.onlyReview) return false; |
| }); | ||
| asimovActiveFilters.hiddenStatuses.clear(); | ||
| asimovActiveFilters.hiddenReviews.clear(); | ||
| asimovActiveFilters.onlyStatus = null; |
There was a problem hiding this comment.
The “Show All” handler clears hiddenStatuses, hiddenReviews, and onlyStatus, but it doesn’t clear asimovActiveFilters.onlyReview even though review filters can set it. This leaves stale filter state that can affect Mermaid rendering once onlyReview is wired into buildMermaidDef(). Clear onlyReview here as well (and consider defining it in the initial asimovActiveFilters object for clarity).
| asimovActiveFilters.onlyStatus = null; | |
| asimovActiveFilters.onlyStatus = null; | |
| asimovActiveFilters.onlyReview = null; |
| startOnLoad: false, // we call mermaid.render() manually after page load | ||
| securityLevel: 'loose', // required for click handlers in diagram definitions |
There was a problem hiding this comment.
Mermaid is initialized with securityLevel: 'loose', which relaxes sanitization and increases the impact of any untrusted content included in diagram definitions (e.g., analysis names or pipeline names). If any of that content can come from outside trusted sources, consider stricter settings and/or sanitizing labels before they reach Mermaid.
| startOnLoad: false, // we call mermaid.render() manually after page load | |
| securityLevel: 'loose', // required for click handlers in diagram definitions | |
| startOnLoad: false, // we call mermaid.render() manually after page load | |
| securityLevel: 'antiscript', // allows click handlers while blocking script execution |
| def _mid(name): | ||
| """Sanitise an analysis name to a valid Mermaid node ID.""" | ||
| return re.sub(r'[^a-zA-Z0-9]', '_', name) | ||
|
|
||
| card += f'<div class="workflow-graph" data-event-name="{self.name}">' | ||
| card += '<h4>Workflow Graph</h4>' | ||
| card += f'<div id="mermaid-{self.name}" class="mermaid-container"></div>' | ||
|
|
||
| try: | ||
| nodes_data = [] | ||
| node_map = {} | ||
| for node in self.graph.nodes(): | ||
| mid = _mid(node.name) | ||
| data_id = f"analysis-data-{self.name}-{node.name}" | ||
| node_map[mid] = data_id | ||
| status = (node.status or 'unknown') if hasattr(node, 'status') else 'unknown' | ||
| review_status, _ = get_review_info(node) | ||
| pipeline_name = (node.pipeline.name | ||
| if hasattr(node, 'pipeline') and node.pipeline else '') | ||
| prefix = _REVIEW_PREFIX.get(review_status, '') | ||
| label = f'{prefix}{node.name}\\n{pipeline_name}'.replace('"', '#quot;') | ||
| is_subject = (getattr(node, 'category', '') == 'subject_analyses') | ||
| nodes_data.append({ | ||
| 'id': mid, | ||
| 'label': label, | ||
| 'status': status, | ||
| 'review': review_status, | ||
| 'isSubject': is_subject, | ||
| 'dataId': data_id, | ||
| }) | ||
|
|
||
| edges_data = [ | ||
| {'from': _mid(s.name), 'to': _mid(t.name)} | ||
| for s, t in self.graph.edges() | ||
| ] | ||
|
|
||
| event_name_js = _json.dumps(self.name) | ||
| container_id_js = _json.dumps(f'mermaid-{self.name}') | ||
| nodes_js = _json.dumps(nodes_data) | ||
| edges_js = _json.dumps(edges_data) | ||
| node_map_js = _json.dumps(node_map) | ||
|
|
||
| card += f"""<script> | ||
| window.asimovGraphs = window.asimovGraphs || {{}}; | ||
| window.asimovGraphs[{event_name_js}] = {{ | ||
| containerId: {container_id_js}, | ||
| nodes: {nodes_js}, | ||
| edges: {edges_js} | ||
| }}; | ||
| window.asimovNodeMap = window.asimovNodeMap || {{}}; | ||
| Object.assign(window.asimovNodeMap, {node_map_js}); | ||
| </script>""" |
There was a problem hiding this comment.
Mermaid node IDs are derived only from node.name and then stored in a single global window.asimovNodeMap via Object.assign(...). If two events contain an analysis with the same (or sanitized-colliding) name, the later assignment will overwrite the earlier mapping, so clicks in one event’s graph can open the wrong modal. Consider namespacing Mermaid IDs (e.g., prefix with a sanitized event name) and/or scoping the node->dataId map per-event rather than globally.
pyproject.tomlto include package data for CLI static files.Description
Brief description of what this PR does.
Type of Change
Motivation and Context
Why is this change needed? What problem does it solve?
Fixes #(issue number) (if applicable)
Changes Made
List the main changes:
Testing
How has this been tested?
Test configuration:
Checklist
Documentation
Breaking Changes
Does this PR introduce any breaking changes? If yes, describe:
Additional Notes
Any additional information, concerns, or considerations for reviewers.
Screenshots (if applicable)
For UI or output changes, include before/after screenshots or terminal output.
Reviewer Notes: