You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refine chart plotting instructions with detailed requirements
Improve error handling and code quality guidelines
Diagram Walkthrough
flowchart LR
A["Chart Template"] --> B["Enhanced Instructions"]
B --> C["Fullscreen Button"]
B --> D["Error Handling"]
C --> E["Plotly Chart with Fullscreen"]
Loading
File Walkthrough
Relevant files
Enhancement
util-chart-plot_instruction.liquid
Enhanced chart plotting template with fullscreen support
The fullscreen fallbacks mention only webkit and ms prefixes; consider including mozRequestFullScreen/mozFullScreen for broader compatibility or confirm it's intentionally excluded.
* 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.
Requiring an inline SVG path icon without allowing minimal styling might render poorly on different mode bar themes; clarify expected viewBox/fill to ensure visibility.
* 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}.
The “no styles” rule conflicts with ensuring the chart fully expands in fullscreen; browsers often require setting width/height:100%. Clarify whether inline styles on the container are permitted during fullscreen.
* 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}.
** You must not create any new html element.
** You must not apply any styles on any html element.
** Keep code compact (few tokens), but fix all errors before returning.
The template simultaneously requires a single <script> block, forbids creating any new HTML elements, and mandates importing Plotly from a CDN—these constraints cannot all be satisfied together. Choose a consistent path: either guarantee Plotly is preloaded (and drop the import requirement) or explicitly allow creating one script element dynamically (or relax the “no new elements” rule) to load Plotly. Without resolving this, generated code will frequently fail or violate requirements, including the new fullscreen behavior.
** Your output must be a single <script>...</script> block with everything needed inside.
** You need to import Plotly.js to plot the charts. The script source should be "https://cdn.plot.ly/plotly-3.0.1.min.js".
** You need to add the MODE bar for each chart you plot.
** You must render the charts on 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.
* Ensure the chart fully expands and scales to the entire screen when fullscreen is active.
... (clipped 4 lines)
Solution Walkthrough:
Before:
// In util-chart-plot_instruction.liquid
***** Hard Requirements *****
** Your output must be a single <script>...</script> block with everything needed inside.
** You need to import Plotly.js to plot the charts. The script source should be "https://cdn.plot.ly/plotly-3.0.1.min.js".
...
** You must not create any new html element.
After:
// OPTION 1: Assume Plotly is pre-loaded by the host environment.
***** Hard Requirements *****
** Your output must be a single <script>...</script> block with everything needed inside.
// The instruction to import Plotly is removed.
...
** You must not create any new html element.
// OPTION 2: Relax the "no new elements" rule.
***** Hard Requirements *****
** Your output must be a single <script>...</script> block with everything needed inside.
** You need to import Plotly.js to plot the charts...
...
** You must not create any new html element, except for what is needed to dynamically load the Plotly.js script.
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical contradiction in the agent's instructions, where it's impossible to import Plotly.js from a CDN while also being restricted to a single <script> block and forbidden from creating new HTML elements.
High
Possible issue
Allow script injection and await load
Loading Plotly within a single script block requires dynamically injecting a script tag; forbidding new elements makes this impossible. Allow a temporary script tag and require waiting for it to load to prevent "Plotly is undefined" errors.
** You need to import Plotly.js to plot the charts. The script source should be "https://cdn.plot.ly/plotly-3.0.1.min.js".
-** You must not create any new html element.+** You must not create any new html element, except a temporary <script> tag injected to load Plotly.js within the single <script> block.+** Ensure the Plotly script has fully loaded (wait for onload or check window.Plotly) before calling any Plotly APIs.
Apply / Chat
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical contradiction in the instructions, where importing Plotly.js inside a single script block is required, but creating new HTML elements (like a dynamic <script> tag) is forbidden. Resolving this makes the instructions feasible.
High
General
Resize chart on fullscreen changes
Specify a concrete action to ensure resizing works: call Plotly.Plots.resize on fullscreen changes (and window resize). This prevents charts from retaining pre-fullscreen dimensions.
-* 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 by calling Plotly.Plots.resize(container) on fullscreenchange (and on window resize).
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion improves a vague instruction by providing a specific implementation detail (Plotly.Plots.resize) to ensure the chart resizes correctly in fullscreen mode, leading to more reliable code generation.
Medium
Add Firefox fullscreen fallbacks
Include Firefox-prefixed Fullscreen API fallbacks to cover older browsers. Add moz-prefixed request/exit methods and the mozfullscreenchange event for better compatibility.
-* 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.+* Use el.requestFullscreen() with fallbacks to el.webkitRequestFullscreen || el.mozRequestFullScreen || el.msRequestFullscreen.+* Exit fullscreen with document.exitFullscreen() and vendor fallbacks (document.webkitExitFullscreen || document.mozCancelFullScreen || document.msExitFullscreen).+* Listen for fullscreenchange, webkitfullscreenchange, mozfullscreenchange, and msfullscreenchange to keep the button working across repeated clicks and ESC exits.
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: This suggestion correctly points out the missing moz prefixes for the Fullscreen API, which improves cross-browser compatibility for the generated code by adding support for older Firefox versions.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Type
Enhancement
Description
Add fullscreen mode bar button to chart plots
Refine chart plotting instructions with detailed requirements
Improve error handling and code quality guidelines
Diagram Walkthrough
File Walkthrough
util-chart-plot_instruction.liquid
Enhanced chart plotting template with fullscreen supportsrc/Plugins/BotSharp.Plugin.ChartHandler/data/agents/6745151e-6d46-4a02-8de4-1c4f21c7da95/templates/util-chart-plot_instruction.liquid