update(arc-basic): highcharts — new implementation#4374
update(arc-basic): highcharts — new implementation#4374github-actions[bot] merged 9 commits intomainfrom
Conversation
New highcharts implementation for arc-basic (Basic Arc Diagram).
There was a problem hiding this comment.
Pull request overview
This PR adds a new highcharts implementation for the arc-basic plot type. The implementation uses Highcharts' arcdiagram series type to visualize character interactions in a story, showing dialogue exchanges as weighted connections between characters arranged along a horizontal axis.
Changes:
- Created new highcharts implementation using raw JSON configuration (since highcharts_core doesn't support arcdiagram type)
- Added metadata YAML file with basic structure for future quality review
- Implemented with Selenium-based screenshot generation and HTML export
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plots/arc-basic/metadata/highcharts.yaml | Metadata file for the highcharts implementation with library version, timestamps, and preview URLs |
| plots/arc-basic/implementations/highcharts.py | Complete highcharts implementation using arcdiagram series type with data generation, styling, and rendering |
Comments suppressed due to low confidence (5)
plots/arc-basic/implementations/highcharts.py:273
- Debug JavaScript code is left in the implementation (lines 255-273). This code prints node information to the console and was likely used during development. Debug code should be removed before merging to production as it adds unnecessary execution time and console output.
# Debug: check SVG node elements
node_info = driver.execute_script("""
var chart = Highcharts.charts[0];
if (!chart || !chart.series[0]) return 'no chart/series';
var nodes = chart.series[0].nodes;
if (!nodes) return 'no nodes property';
var info = 'nodes: ' + nodes.length;
nodes.forEach(function(n, i) {
if (n.graphic) {
var el = n.graphic.element;
info += '\\n ' + i + ': tag=' + el.tagName + ' attrs=';
for (var a of el.attributes) info += a.name + '=' + a.value + ' ';
} else {
info += '\\n ' + i + ': no graphic';
}
});
return info;
""")
print(node_info)
plots/arc-basic/implementations/highcharts.py:73
- The weight values are scaled up by a factor of 8 (line 72), but this appears arbitrary and the comment suggests this is to work around how arc diagram sizes nodes. Consider if there's a more direct way to configure node sizing in Highcharts rather than manipulating the underlying data values, as this makes the data representation less transparent.
# Scale weights up for visual node sizing (arc diagram sizes nodes by total weight flow)
weight_scale = 8
links_data = [{"from": src, "to": tgt, "weight": w * weight_scale} for src, tgt, w in edges]
plots/arc-basic/implementations/highcharts.py:199
- The implementation includes complex post-render DOM manipulation JavaScript (lines 173-199) that removes link labels and modifies node circles. This suggests the Highcharts arcdiagram configuration isn't fully meeting requirements declaratively. While this may be necessary given library limitations, it increases maintenance complexity and fragility. Consider documenting why this workaround is needed and whether there are alternative Highcharts configuration options that could reduce or eliminate the need for DOM manipulation.
cleanup_js = """
setTimeout(function() {
// Remove link labels (internal IDs rendered on arc paths)
document.querySelectorAll('.highcharts-data-label textPath').forEach(function(el) {
el.parentNode.parentNode.style.display = 'none';
});
document.querySelectorAll('.highcharts-data-label text').forEach(function(el) {
if (el.textContent && el.textContent.indexOf('highcharts-') === 0) {
el.parentNode.style.display = 'none';
}
});
// Enlarge node circles for 4800x2700 canvas
var chart = Highcharts.charts[0];
if (chart && chart.series[0] && chart.series[0].nodes) {
chart.series[0].nodes.forEach(function(node) {
if (node.graphic) {
var el = node.graphic.element;
if (el.tagName === 'circle') {
el.setAttribute('r', 20);
el.setAttribute('stroke', '#ffffff');
el.setAttribute('stroke-width', '4');
}
}
});
}
}, 1500);
"""
plots/arc-basic/implementations/highcharts.py:167
- The code downloads Highcharts JavaScript files with retry logic (5 attempts with exponential backoff), but this only catches HTTPError exceptions. Network errors (URLError, timeout) and other exceptions will still cause the script to fail without retries. Consider catching broader exceptions or at least urllib.error.URLError to make the retry logic more robust.
for attempt in range(5):
try:
with urllib.request.urlopen(url, timeout=30) as resp:
content = resp.read().decode("utf-8")
cache_path.write_text(content, encoding="utf-8")
js_scripts[name] = content
break
except urllib.error.HTTPError:
time.sleep(3 * (attempt + 1))
plots/arc-basic/metadata/highcharts.yaml:9
- The library version in metadata (1.10.3) refers to the Python highcharts_core package version, but this implementation doesn't use highcharts_core. Instead, it downloads JavaScript from CDN at version 11.4.8. This creates a mismatch between the metadata and the actual implementation. The library_version field should reflect the actual Highcharts JavaScript library version being used (11.4.8), not a hypothetical Python package version.
library_version: 1.10.3
| "nodeFormat": "{point.name}: {point.sum} exchanges", | ||
| "pointFormat": "{point.fromNode.name} \u2192 {point.toNode.name}: {point.weight} exchanges", |
There was a problem hiding this comment.
The tooltip displays scaled weight values (line 100), not the original edge weights. Since weights are multiplied by 8 on line 72, a tooltip showing an edge with original weight 5 will display "40 exchanges" instead of "5 exchanges", which misrepresents the actual data. Either avoid scaling the data or use a tooltip formatter that divides by the scale factor to show true values.
| js_scripts[name] = content | ||
| break | ||
| except urllib.error.HTTPError: | ||
| time.sleep(3 * (attempt + 1)) |
There was a problem hiding this comment.
If all 5 download retry attempts fail (line 159-167), the code continues execution and will crash at lines 168-170 when accessing dictionary keys that don't exist in js_scripts. The retry loop should either raise an exception after exhausting retries or check if the download succeeded before proceeding.
| time.sleep(3 * (attempt + 1)) | |
| time.sleep(3 * (attempt + 1)) | |
| else: | |
| raise RuntimeError(f"Failed to download '{name}' from {url} after 5 attempts") |
AI Review - Attempt 1/3Image Description
Score: 82/100
Visual Quality (25/30)
Design Excellence (12/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (9/10)
Library Mastery (7/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
Attempt 1/3 - fixes based on AI review
🔧 Repair Attempt 1/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
AI Review - Attempt 2/3Image Description
Score: 87/100
Visual Quality (27/30)
Design Excellence (15/20)
Spec Compliance (14/15)
Data Quality (14/15)
Code Quality (10/10)
Library Mastery (7/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
Attempt 2/3 - fixes based on AI review
🔧 Repair Attempt 2/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
AI Review - Attempt 3/3Image Description
Score: 85/100
Visual Quality (26/30)
Design Excellence (14/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (9/10)
Library Mastery (7/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
Attempt 3/3 - fixes based on AI review
🔧 Repair Attempt 3/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
AI Review - Attempt 3/3Image Description
Score: 87/100
Visual Quality (26/30)
Design Excellence (14/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (10/10)
Library Mastery (8/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
Summary
New highcharts implementation for arc-basic.
Changes
Test Plan
Generated with Claude Code
/updatecommand