Skip to content

Conversation

Joannall
Copy link
Collaborator

@Joannall Joannall commented Sep 4, 2025

PR Type

Enhancement


Description

  • Add report summary feature to chart plotting functionality

  • Implement delayed message sending for chart insights

  • Expose EventEmitter class for external usage

  • Update LLM response format to include summary reports


Diagram Walkthrough

flowchart LR
  A["Chart Plot Request"] --> B["Generate Chart Code"]
  B --> C["Send Chart to User"]
  C --> D["Generate Report Summary"]
  D --> E["Send Delayed Summary Message"]
Loading

File Walkthrough

Relevant files
Enhancement
PlotChartFn.cs
Enhanced chart function with delayed summary messaging     

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

  • Add BotSharpOptions dependency injection
  • Implement delayed message sending for report summaries
  • Add SendDelayedMessage and SendEvent helper methods
  • Include error handling for delayed message operations
+59/-1   
LlmContextOut.cs
Add report summary property to LLM context                             

src/Plugins/BotSharp.Plugin.ChartHandler/LlmContext/LlmContextOut.cs

  • Add ReportSummary property to LLM output model
  • Include JSON serialization attributes for new property
+4/-0     
EventEmitter.cs
Expose EventEmitter class for external usage                         

src/Plugins/BotSharp.Plugin.ChatHub/Helpers/EventEmitter.cs

  • Change class visibility from internal to public
  • Change SendChatEvent method visibility to public
+2/-2     
util-chart-plot_instruction.liquid
Update LLM template for report summary generation               

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

  • Update LLM instruction template to include report_summary field
  • Specify markdown format for insight summary reports
+2/-1     
Dependencies
BotSharp.Plugin.ChartHandler.csproj
Add ChatHub plugin dependency                                                       

src/Plugins/BotSharp.Plugin.ChartHandler/BotSharp.Plugin.ChartHandler.csproj

  • Add project reference to BotSharp.Plugin.ChatHub
+1/-0     

Copy link

qodo-merge-pro bot commented Sep 4, 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 Usage

In SendDelayedMessage, the conversation storage is obtained via GetService and used with a null-conditional. If storage is null, the message won't be persisted but an event is still emitted. Validate that a missing storage is acceptable or add a guard/log for this case.

    var storage = services.GetService<IConversationStorage>();
    storage?.Append(conversationId, dialogModel);
    await SendEvent(services, ChatEvent.OnMessageReceivedFromAssistant, conversationId, messageData);

}
Background Task Lifetime

The delayed Task.Run uses a scoped service provider created from _services; if the host is shutting down or scope services have limited lifetime, this may race with disposal. Consider using a background job scheduler or IHostedService, and flowing CancellationToken to Task.Delay and SendEvent.

    _ = Task.Run(async () =>
    {
        var services = _services.CreateScope().ServiceProvider;
        await Task.Delay(1500);
        await SendDelayedMessage(services, obj.ReportSummary, convService.ConversationId, agent.Id, agent.Name);
    });
}
JSON Options Dependency

SendEvent serializes with _options.JsonSerializerOptions. Ensure BotSharpOptions is registered and options are non-null; otherwise add null-check or fallback to default JsonSerializerOptions.

var json = JsonSerializer.Serialize(data, _options.JsonSerializerOptions);
await EventEmitter.SendChatEvent(services, _logger, @event, conversationId, user?.Id, json, nameof(PlotChartFn), callerName);

Copy link

qodo-merge-pro bot commented Sep 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Dispose scope in background task

Dispose the created service scope to avoid leaking scoped services in the
background task. Also wrap the entire background operation in a try/catch to
ensure exceptions are observed and logged. This prevents unobserved task
exceptions and resource leaks.

src/Plugins/BotSharp.Plugin.ChartHandler/Functions/PlotChartFn.cs [75-84]

 // Send report summary after 1.5 seconds if exists
 if (!string.IsNullOrEmpty(obj?.ReportSummary))
 {
     _ = Task.Run(async () =>
     {
-        var services = _services.CreateScope().ServiceProvider;
-        await Task.Delay(1500);
-        await SendDelayedMessage(services, obj.ReportSummary, convService.ConversationId, agent.Id, agent.Name);
+        try
+        {
+            using var scope = _services.CreateScope();
+            await Task.Delay(1500);
+            await SendDelayedMessage(scope.ServiceProvider, obj.ReportSummary, convService.ConversationId, agent.Id, agent.Name);
+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, "Failed to schedule delayed message");
+        }
     });
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a resource leak where the IServiceScope created by _services.CreateScope() is not disposed, and also points out the risk of unobserved exceptions in a fire-and-forget task. Applying this change is crucial for application stability and resource management.

Medium
Send typed payload, not JSON string

Avoid manual JSON serialization before sending the SignalR event. Sending a
pre-serialized string can break clients expecting a structured payload and leads
to double serialization. Pass the typed object directly and let SignalR handle
serialization.

src/Plugins/BotSharp.Plugin.ChartHandler/Functions/PlotChartFn.cs [121-126]

 private async Task SendEvent<T>(IServiceProvider services, string @event, string conversationId, T data, [CallerMemberName] string callerName = "")
 {
     var user = services.GetService<IUserIdentity>();
-    var json = JsonSerializer.Serialize(data, _options.JsonSerializerOptions);
-    await EventEmitter.SendChatEvent(services, _logger, @event, conversationId, user?.Id, json, nameof(PlotChartFn), callerName);
+    await EventEmitter.SendChatEvent(services, _logger, @event, conversationId, user?.Id, data, nameof(PlotChartFn), callerName);
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that manually serializing the payload to a JSON string before passing it to EventEmitter.SendChatEvent is unnecessary and can lead to double serialization issues. Passing the object directly and letting the underlying framework (SignalR) handle serialization is better practice.

Medium
  • More

@iceljc
Copy link
Collaborator

iceljc commented Sep 5, 2025

I will refine this tomorrow.


<ItemGroup>
<ProjectReference Include="..\..\Infrastructure\BotSharp.Core\BotSharp.Core.csproj" />
<ProjectReference Include="..\BotSharp.Plugin.ChatHub\BotSharp.Plugin.ChatHub.csproj" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't reference ChatHub in Core project.

@Oceania2018 Oceania2018 merged commit 27ada7c into SciSharp:master Sep 5, 2025
3 of 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.

3 participants