Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Sep 5, 2025

PR Type

Enhancement


Description

  • Add chart data and code generation endpoints

  • Implement SQL-based chart service with LLM integration

  • Create abstraction layer for chart service providers

  • Update ECharts template with fullscreen functionality


Diagram Walkthrough

flowchart LR
  A["Chart Abstraction Layer"] --> B["Chart Service Interface"]
  B --> C["SQL Chart Service"]
  C --> D["LLM Code Generation"]
  E["API Endpoints"] --> A
  F["Chart Templates"] --> D
Loading

File Walkthrough

Relevant files
Enhancement
13 files
IBotSharpChartService.cs
Define chart service interface                                                     
+14/-0   
ChartCodeOptions.cs
Add chart code generation options model                                   
+25/-0   
ChartCodeResult.cs
Add chart code result model                                                           
+7/-0     
ChartDataOptions.cs
Add chart data options model                                                         
+9/-0     
ChartDataResult.cs
Add chart data result model                                                           
+6/-0     
ConversationController.cs
Add chart data and code endpoints                                               
+30/-0   
Using.cs
Add chart models global using                                                       
+1/-0     
ConversationChartDataRequest.cs
Add chart request view models                                                       
+17/-0   
ConversationChartDataResponse.cs
Add chart response view models                                                     
+40/-0   
ChartLlmContextOut.cs
Add chart LLM context output model                                             
+18/-0   
SqlChartService.cs
Implement SQL-based chart service                                               
+155/-0 
Using.cs
Add chart abstractions using statements                                   
+2/-0     
util-chart-plot_instruction.liquid
Update ECharts template with fullscreen support                   
+3/-2     
Configuration changes
2 files
SqlDriverPlugin.cs
Register chart service in DI                                                         
+1/-0     
appsettings.json
Enable SqlDriver plugin in configuration                                 
+1/-0     
Formatting
1 files
BotSharp.Plugin.SqlDriver.csproj
Update project file encoding                                                         
+1/-1     

Copy link

qodo-merge-pro bot commented Sep 5, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

Authorization/Information disclosure:
The GET chart data endpoint is marked AllowAnonymous and the POST chart code endpoint has no explicit authorization attributes. If not gated elsewhere, unauthenticated callers could access conversation-derived data or trigger LLM code generation. Validate that conversation/message access is enforced by middleware or add appropriate [Authorize] with scope checks.

Remote code/content injection risk: The LLM-generated JavaScript is returned to clients (Language: "javascript") and the template instructs importing external ECharts from a CDN. If rendered directly, this can enable XSS or supply-chain risks. Consider sanitizing/isolating execution (e.g., sandboxed iframe), restricting allowed domains via CSP, and avoiding arbitrary external script injection by serving a pinned, locally hosted library.

No SQL injection concern apparent in the diff, but verify any future use of TargetStateName/States when constructing queries.

⚡ Recommended focus areas for review

Possible Nulls

ChartCodeResult fields Code/Language and ChartDataResult Data are non-nullable but assigned from possibly null sources; this can cause null reference issues at runtime or serialization inconsistencies. Consider making these properties nullable or enforcing non-null defaults/validation.

var obj = response.JsonContent<ChartLlmContextOut>();

return new ChartCodeResult
{
    Code = obj?.JsCode,
    Language = "javascript"
};
Dependency Mismatch

Template requires ECharts 6.0.0 via CDN; verify compatibility with existing chart code and hosting CSP. The CDN/version change from 5.5.1 may break features or icons and should be validated.

You must strictly follow the "Hard Requirements", "Render Requirements", "Code Requirements" and "Response Format" below.

=== Plotting Requirement ===
{{ plotting_requirement }}


***** 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 exactly once to plot the charts. The script source is "https://cdnjs.cloudflare.com/ajax/libs/echarts/6.0.0/echarts.min.js".
** You must add an ECharts Toolbox bar at the top left corner of the chart.
** ALWAYS add a "Full screen" button right next to the Toolbox bar.
** The "Full screen" button can toggle 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.
    * When entering fullscreen, the chart container must have widhth: 100vw and height: 100vh.
    * 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.
    * fullscreenBtn must be a fully-formed object {show: true, name, title, icon: 'path://M3 3 H9 V5 H5 V9 H3 Z M15 3 H21 V9 H19 V5 H15 Z M3 15 H5 V19 H9 V21 H3 Z M19 15 H21 V21 H15 V19 H19 Z', onclick}.
    * When using "chart.setOption" to define the fullscreen button, DO NOT use "graphic". Include the fullscreenBtn object in toolbox.feature with name 'myFullscreen'.
** Initialize the chart with explicit non-zero width (at least 800px) and non-zero height (at least 500px).
Missing AuthN/AuthZ

Chart data endpoint is AllowAnonymous and chart code endpoint lacks explicit authorization checks; ensure this exposure is intentional and that responses don’t leak conversation data across tenants.

[AllowAnonymous]
[HttpGet("/conversation/{conversationId}/message/{messageId}/user/chart/data")]
public async Task<ConversationChartDataResponse?> GetConversationChartData(
    [FromRoute] string conversationId,
    [FromRoute] string messageId,
    [FromQuery] ConversationChartDataRequest request)
{
    var chart = _services.GetServices<IBotSharpChartService>().FirstOrDefault(x => x.Provider == request?.ChartProvider);
    if (chart == null) return null;

    var result = await chart.GetConversationChartData(conversationId, messageId, request);
    return ConversationChartDataResponse.From(result);
}

[HttpPost("/conversation/{conversationId}/message/{messageId}/user/chart/code")]
public async Task<ConversationChartCodeResponse?> GetConversationChartCode(
    [FromRoute] string conversationId,
    [FromRoute] string messageId,
    [FromBody] ConversationChartCodeRequest request)
{
    var chart = _services.GetServices<IBotSharpChartService>().FirstOrDefault(x => x.Provider == request?.ChartProvider);
    if (chart == null) return null;

    var result = await chart.GetConversationChartCode(conversationId, messageId, request);
    return ConversationChartCodeResponse.From(result);
}
#endregion

Copy link

qodo-merge-pro bot commented Sep 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Secure the chart data endpoint

Exposing conversation chart data publicly can leak sensitive user information.
Remove anonymous access so the controller-level authorization applies. This
ensures only authenticated users can retrieve chart data.

src/Infrastructure/BotSharp.OpenAPI/Controllers/ConversationController.cs [535-540]

-[AllowAnonymous]
 [HttpGet("/conversation/{conversationId}/message/{messageId}/user/chart/data")]
 public async Task<ConversationChartDataResponse?> GetConversationChartData(
     [FromRoute] string conversationId,
     [FromRoute] string messageId,
     [FromQuery] ConversationChartDataRequest request)
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical security vulnerability by pointing out that the [AllowAnonymous] attribute exposes sensitive conversation data publicly.

High
Possible issue
Fix invalid ECharts CDN URL

The CDN URL points to a non-existent ECharts version, causing charts to fail to
render. Revert to a stable, existing version to ensure the generated code loads
the library successfully. Use the previously working 5.5.1 build.

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

-** You need to import ECharts.js exactly once 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 exactly once to plot the charts. The script source is "https://cdnjs.cloudflare.com/ajax/libs/echarts/5.5.1/echarts.min.js".
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that the ECharts CDN URL for version 6.0.0 is invalid, which would break the chart rendering functionality.

High
Use configured LLM defaults

Defaulting to an invalid model like "gpt-5" will cause runtime failures when no
overrides are provided. Derive defaults from configured AgentSettings and then
let request overrides take precedence. This prevents hard-to-diagnose
provider/model errors.

src/Plugins/BotSharp.Plugin.SqlDriver/Services/SqlChartService.cs [142-154]

 private (string, string) GetLlmProviderModel(ChartCodeOptions options)
 {
-    var provider = "openai";
-    var model = "gpt-5";
+    // Use configured defaults from AgentSettings to avoid invalid model names
+    var agentSettings = _services.GetRequiredService<BotSharp.Abstraction.Agents.Settings.AgentSettings>();
+    var provider = agentSettings.LlmConfig.Provider;
+    var model = agentSettings.LlmConfig.Model;
 
     if (options?.Llm != null)
     {
         provider = options.Llm.Provider.IfNullOrEmptyAs(provider);
         model = options.Llm.Model.IfNullOrEmptyAs(model);
     }
 
     return (provider, model);
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that hardcoding a non-existent model like gpt-5 is a bug and proposes a more robust solution by using configured defaults.

Medium
  • More

@iceljc iceljc merged commit 7fda8d2 into SciSharp:master Sep 5, 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.

1 participant