Skip to content

Conversation

Joannall
Copy link
Collaborator

@Joannall Joannall commented Sep 2, 2025

PR Type

Enhancement


Description

  • Switch chart library from Plotly.js to ECharts.js

  • Add LLM configuration with 8192 max output tokens

  • Enhance chart template with fullscreen functionality

  • Update plotting requirements and error handling


Diagram Walkthrough

flowchart LR
  A["PlotChartFn.cs"] --> B["Add LlmConfig"]
  A --> C["Minor formatting fix"]
  D["util-chart-plot_instruction.liquid"] --> E["Switch to ECharts.js"]
  D --> F["Add fullscreen button"]
  D --> G["Enhanced requirements"]
Loading

File Walkthrough

Relevant files
Enhancement
PlotChartFn.cs
Add LLM configuration and formatting fix                                 

src/Plugins/BotSharp.Plugin.ChartHandler/Functions/PlotChartFn.cs

  • Add LlmConfig with MaxOutputTokens set to 8192
  • Fix minor indentation formatting issue
+5/-1     
util-chart-plot_instruction.liquid
Switch to ECharts.js with fullscreen functionality             

src/Plugins/BotSharp.Plugin.ChartHandler/data/agents/6745151e-6d46-4a02-8de4-1c4f21c7da95/templates/util-chart-plot_instruction.liquid

  • Replace Plotly.js with ECharts.js library
  • Add fullscreen mode bar button with Fullscreen API
  • Enhance plotting requirements and error handling
  • Update response format and validation rules
+16/-7   

Copy link

qodo-merge-pro bot commented Sep 2, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect Library Instructions

The template still references Plotly-specific mode bar API and usage (e.g., Plotly.newPlot, modeBarButtonsToAdd, Plotly SVG) while the library has been switched to ECharts, which has a different API and no Plotly-style mode bar. Instructions should be converted to ECharts equivalents or custom UI handling.

** Add a custom mode bar button named "Fullscreen" that toggles the chart container in and out of fullscreen using the Fullscreen API. Requirements for this button:
    * Always call the Fullscreen API on the chart container div itself (document.getElementById("{{ chart_element_id }}")), not on the document or on Plotly’s SVG.
    * Use el.requestFullscreen() with fallbacks to el.webkitRequestFullscreen || el.msRequestFullscreen.
    * Exit fullscreen with document.exitFullscreen() and vendor fallbacks.
    * Listen for fullscreenchange, webkitfullscreenchange, and msfullscreenchange to keep the button working across repeated clicks and ESC exits.
    * Ensure the chart fully expands and scales to the entire screen when fullscreen is active.
    * Provide a simple inline SVG path icon for the button (no external assets).
    * Use Plotly.newPlot(container, data, layout, {displayModeBar:true, modeBarButtonsToAdd:[fullscreenBtn]});
    * fullscreenBtn must be a fully-formed object {name, title, icon, click}.
Script Tag Multiplicity

The new hard requirement allows multiple <script> blocks, while the previous contract enforced a single block. Verify downstream consumers and validators accept multiple blocks; otherwise, rendering or parsing may break.

***** Hard Requirements *****
** Your output javascript code must be wrapped in one or multiple <script>...</script> blocks with everything needed inside.
** You need to import ECharts.js to plot the charts. The script source is "https://cdnjs.cloudflare.com/ajax/libs/echarts/6.0.0/echarts.min.js".
Large Max Tokens

Increasing MaxOutputTokens to 8192 may cause longer latency and higher costs; ensure the model/back-end supports this limit and that downstream parsing of larger JSON payloads remains robust.

LlmConfig = new AgentLlmConfig
{
    MaxOutputTokens = 8192
},

Copy link

qodo-merge-pro bot commented Sep 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Fix ECharts template inconsistencies

The instruction still references Plotly APIs (mode bar, Plotly.newPlot,
modeBarButtonsToAdd, SVG) and uses a likely non-existent ECharts CDN version, so
the LLM will produce code that doesn’t run. Rewrite the template to fully align
with ECharts (echarts.init/setOption and a toolbox custom fullscreen feature),
remove Plotly-specific terms, and point to a valid ECharts 5.x CDN. Also clarify
container sizing or relax the “no styles” rule so charts don’t render at zero
size and fullscreen behaves correctly.

Examples:

src/Plugins/BotSharp.Plugin.ChartHandler/data/agents/6745151e-6d46-4a02-8de4-1c4f21c7da95/templates/util-chart-plot_instruction.liquid [6-24]
***** Hard Requirements *****
** Your output javascript code must be wrapped in one or multiple <script>...</script> blocks with everything needed inside.
** You need to import ECharts.js to plot the charts. The script source is "https://cdnjs.cloudflare.com/ajax/libs/echarts/6.0.0/echarts.min.js".
** You need to add the MODE bar for each chart you plot.
** You must render the charts under the div html element with id {{ chart_element_id }}.
** Add a custom mode bar button named "Fullscreen" that toggles the chart container in and out of fullscreen using the Fullscreen API. Requirements for this button:
    * Always call the Fullscreen API on the chart container div itself (document.getElementById("{{ chart_element_id }}")), not on the document or on Plotly’s SVG.
    * Use el.requestFullscreen() with fallbacks to el.webkitRequestFullscreen || el.msRequestFullscreen.
    * Exit fullscreen with document.exitFullscreen() and vendor fallbacks.
    * Listen for fullscreenchange, webkitfullscreenchange, and msfullscreenchange to keep the button working across repeated clicks and ESC exits.

 ... (clipped 9 lines)

Solution Walkthrough:

Before:

***** Hard Requirements *****
** You need to import ECharts.js ... The script source is ".../echarts/6.0.0/echarts.min.js".
** You need to add the MODE bar for each chart you plot.
** Add a custom mode bar button named "Fullscreen"...
...
* Use Plotly.newPlot(container, data, layout, {displayModeBar:true, modeBarButtonsToAdd:[fullscreenBtn]});
* fullscreenBtn must be a fully-formed object {name, title, icon, click}.
** You must not apply any styles on any html element.
** Do not generate charts with zero height and zero width.

After:

***** Hard Requirements *****
** You need to import ECharts.js ... The script source should be a valid version, e.g., ".../echarts/5.5.0/echarts.min.js".
** Render charts in the div with id {{ chart_element_id }}, ensuring it has a size (e.g., by adding style="width:100%;height:400px").
** Initialize charts using `echarts.init(...)` and configure with `chart.setOption(...)`.
** Add a fullscreen feature using the ECharts `toolbox` component with a custom tool.
toolbox: {
  feature: {
    myTool: {
      show: true,
      title: 'Fullscreen',
      icon: '...', // SVG path for icon
      onclick: function () { /* Fullscreen API logic */ }
    }
  }
}
Suggestion importance[1-10]: 10

__

Why: This suggestion correctly identifies multiple critical flaws, such as using Plotly-specific APIs (Plotly.newPlot) in an ECharts template and an invalid CDN URL, which would render the entire feature non-functional.

High
Possible issue
Align instructions with ECharts toolbox

The instructions mix Plotly-specific features with ECharts, causing unexecutable
output. Replace the Plotly "mode bar" and newPlot guidance with ECharts' toolbox
and custom feature callback so the LLM can generate valid ECharts code. Keep the
fullscreen requirements but apply them to the container and call chart.resize()
on fullscreen changes.

src/Plugins/BotSharp.Plugin.ChartHandler/data/agents/6745151e-6d46-4a02-8de4-1c4f21c7da95/templates/util-chart-plot_instruction.liquid [9-19]

-** You need to add the MODE bar for each chart you plot.
-** You must render the charts under the div html element with id {{ chart_element_id }}.
-** Add a custom mode bar button named "Fullscreen" that toggles the chart container in and out of fullscreen using the Fullscreen API. Requirements for this button:
-    * Always call the Fullscreen API on the chart container div itself (document.getElementById("{{ chart_element_id }}")), not on the document or on Plotly’s SVG.
+** Enable the ECharts toolbox for each chart you plot.
+** Render the chart on the div html element with id {{ chart_element_id }} (use it as the ECharts container; do not create children).
+** Add a custom toolbox button named "Fullscreen" that toggles the chart container in and out of fullscreen using the Fullscreen API. Requirements for this button:
+    * Always call the Fullscreen API on the chart container div itself (document.getElementById("{{ chart_element_id }}")), not on the document or inner canvas.
     * Use el.requestFullscreen() with fallbacks to el.webkitRequestFullscreen || el.msRequestFullscreen.
     * Exit fullscreen with document.exitFullscreen() and vendor fallbacks.
     * Listen for fullscreenchange, webkitfullscreenchange, and msfullscreenchange to keep the button working across repeated clicks and ESC exits.
-    * Ensure the chart fully expands and scales to the entire screen when fullscreen is active.
+    * Ensure the chart fully expands and scales to the entire screen when fullscreen is active; call chart.resize() on relevant fullscreen events.
     * Provide a simple inline SVG path icon for the button (no external assets).
-    * Use Plotly.newPlot(container, data, layout, {displayModeBar:true, modeBarButtonsToAdd:[fullscreenBtn]});
-    * fullscreenBtn must be a fully-formed object {name, title, icon, click}.
+    * Configure an ECharts toolbox feature, e.g. option.toolbox = { show: true, feature: { myFullscreen: { show: true, title: 'Fullscreen', icon: 'path://M3,3 L9,3 L9,5 L5,5 L5,9 L3,9 Z M15,15 L9,15 L9,13 L13,13 L13,9 L15,9 Z', onclick: function(){ /* toggle fullscreen on the container and then chart.resize() */ } } } }; Initialize with const chart = echarts.init(container); chart.setOption(option).
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies that the PR mixes instructions for ECharts.js with Plotly.js-specific APIs (Plotly.newPlot, mode bar), which would lead to incorrect code generation. This is a critical fix for the functionality.

High
Use a valid ECharts CDN URL

The specified ECharts CDN URL/version may not exist and will break chart
loading. Use a stable, widely-available CDN path for ECharts to ensure the
script can be fetched reliably.

src/Plugins/BotSharp.Plugin.ChartHandler/data/agents/6745151e-6d46-4a02-8de4-1c4f21c7da95/templates/util-chart-plot_instruction.liquid [8]

-** You need to import ECharts.js to plot the charts. The script source is "https://cdnjs.cloudflare.com/ajax/libs/echarts/6.0.0/echarts.min.js".
+** You need to import ECharts.js to plot the charts. The script source is "https://cdn.jsdelivr.net/npm/echarts/dist/echarts.min.js".
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that the provided CDN URL for ECharts is invalid and would cause the chart generation to fail. Providing a valid URL is crucial for the feature to work correctly.

Medium
  • Update

@Oceania2018 Oceania2018 merged commit 3dba483 into SciSharp:master Sep 3, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants