Skip to content

refine chart plot template #1142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
iceljc merged 1 commit into from
Sep 2, 2025
Merged

refine chart plot template #1142

iceljc merged 1 commit into from
Sep 2, 2025

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Sep 2, 2025 •
edited by qodo-code-review bot
Loading

PR Type

Enhancement

Description

  • Add dynamic chart plot template selection

  • Extract template retrieval logic into separate method

  • Support custom templates via conversation state

Diagram Walkthrough

flowchart LR
  A["Execute Method"] --> B["GetChartPlotInstruction"]
  B --> C["Check State Service"]
  C --> D["Custom Template?"]
  D -->|Yes| E["Use Agent Template"]
  D -->|No| F["Use Default Template"]
  E --> G["Return Template Content"]
  F --> G
Loading

File Walkthrough
Relevant files
Enhancement
PlotChartFn.cs
Dynamic chart plot template selection implementation         

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

  • Removed direct database call from Execute method
  • Added GetChartPlotInstruction method for template retrieval
  • Implemented state-based template selection logic
  • Added fallback to default utility assistant template
 +23/-2   

@iceljc iceljc merged commit f661a39 into SciSharp:master Sep 2, 2025
0 of 4 checks passed
@qodo-code-review
Copy link

qodo-code-review 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

Possible Null/Empty Template

GetChartPlotInstruction may return an empty string if the template lookup fails (missing custom template name or missing default). Downstream usage of an empty instruction may degrade LLM outputs silently; consider validating and logging/falling back to a hardcoded default.

 
private string GetChartPlotInstruction(string agentId)
{
    var db = _services.GetRequiredService<IBotSharpRepository>();
    var state = _services.GetRequiredService<IConversationStateService>();

    var templateContent = string.Empty;
    var templateName = state.GetState("chart_plot_template");

    if (!string.IsNullOrEmpty(templateName))
    {
        templateContent = db.GetAgentTemplate(agentId, templateName);
    }
    else
    {
        templateName = "util-chart-plot_instruction";
        templateContent = db.GetAgentTemplate(BuiltInAgentId.UtilityAssistant, templateName);
    }

    return templateContent;
}
Missing Async Call

GetChartPlotInstruction accesses services synchronously; if repository/state services offer async equivalents, consider async to avoid blocking. Also, potential I/O failures are not handled; add try/catch or null checks.

 
private string GetChartPlotInstruction(string agentId)
{
    var db = _services.GetRequiredService<IBotSharpRepository>();
    var state = _services.GetRequiredService<IConversationStateService>();

    var templateContent = string.Empty;
    var templateName = state.GetState("chart_plot_template");

    if (!string.IsNullOrEmpty(templateName))
    {
        templateContent = db.GetAgentTemplate(agentId, templateName);
    }
    else
    {
        templateName = "util-chart-plot_instruction";
        templateContent = db.GetAgentTemplate(BuiltInAgentId.UtilityAssistant, templateName);
    }

    return templateContent;
}

@qodo-code-review
Copy link

qodo-code-review bot commented Sep 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add reliable template fallback

Add a robust fallback when the agent-specific template is missing or resolves to
empty content. If both the agent-specific and default templates cannot be
loaded, log a warning and provide a minimal safe instruction to avoid proceeding
with an empty prompt. Trim the state value to prevent issues from
whitespace-only names.

src/Plugins/BotSharp.Plugin.ChartHandler/Functions/PlotChartFn.cs [83-102]

 private string GetChartPlotInstruction(string agentId)
 {
     var db = _services.GetRequiredService<IBotSharpRepository>();
     var state = _services.GetRequiredService<IConversationStateService>();
 
-    var templateContent = string.Empty;
-    var templateName = state.GetState("chart_plot_template");
+    var templateName = state.GetState("chart_plot_template")?.Trim();
+    string templateContent = null;
 
     if (!string.IsNullOrEmpty(templateName))
     {
         templateContent = db.GetAgentTemplate(agentId, templateName);
+        if (string.IsNullOrWhiteSpace(templateContent))
+        {
+            templateContent = db.GetAgentTemplate(BuiltInAgentId.UtilityAssistant, "util-chart-plot_instruction");
+        }
     }
     else
     {
-        templateName = "util-chart-plot_instruction";
-        templateContent = db.GetAgentTemplate(BuiltInAgentId.UtilityAssistant, templateName);
+        templateContent = db.GetAgentTemplate(BuiltInAgentId.UtilityAssistant, "util-chart-plot_instruction");
+    }
+
+    if (string.IsNullOrWhiteSpace(templateContent))
+    {
+        _logger.LogWarning("Chart plot template not found for agent {AgentId}. Falling back to minimal instruction.", agentId);
+        templateContent = "Generate chart plotting instructions with clear axes, labels, data mappings, and chart type.";
     }
 
     return templateContent;
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the new GetChartPlotInstruction method could return an empty string if a template name exists but its content is empty, and improves robustness by adding multiple fallback layers and logging.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@iceljc