Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Sep 18, 2025

PR Type

Enhancement


Description

  • Add intermediate message support for chat events

  • Refactor database configuration in ExcelHandler plugin

  • Remove dependency on SqlDriver plugin

  • Update file selection model structure


Diagram Walkthrough

flowchart LR
  A["Chat Events"] --> B["Intermediate Messages"]
  C["ExcelHandler"] --> D["Database Config"]
  C --> E["Remove SqlDriver Dependency"]
  F["File Models"] --> G["Simplified Structure"]
Loading

File Walkthrough

Relevant files
Enhancement
8 files
ChatEvent.cs
Add intermediate message chat event                                           
+1/-0     
FileSelectContext.cs
Remove filename property from FileSelectItem                         
+0/-3     
HubObserveData.cs
Add SaveDataToDb property with documentation                         
+8/-0     
GetWeatherFn.cs
Add intermediate message emission in debug mode                   
+11/-0   
FileInstructService.Pdf.cs
Refactor PDF to image conversion logic                                     
+9/-8     
FileInstructService.cs
Simplify image converter retrieval method                               
+1/-3     
ExecuteTemplateFn.cs
Remove instruction hook emission code                                       
+0/-11   
ChatHubObserver.cs
Add intermediate message handling with database save         
+26/-0   
Miscellaneous
2 files
EventEmitter.cs
Change class visibility to internal                                           
+2/-2     
Using.cs
Add ExcelHandlerSettings to global usings                               
+1/-0     
Configuration changes
6 files
ReadExcelFn.cs
Update database provider configuration access                       
+1/-2     
MySqlService.cs
Replace SqlDriver dependency with local settings                 
+7/-6     
SqliteService.cs
Replace SqlDriver dependency with local settings                 
+5/-4     
ExcelHandlerSettings.cs
Add DatabaseSettings class with provider and connection   
+7/-1     
SqlDriverSetting.cs
Remove temporary connection string properties                       
+0/-2     
appsettings.json
Update ExcelHandler configuration structure                           
+4/-1     
Documentation
1 files
instruction.liquid
Update router instruction with default language                   
+1/-1     
Dependencies
1 files
BotSharp.Plugin.ExcelHandler.csproj
Remove SqlDriver project reference                                             
+0/-1     

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Possible Issue

BuildFileName composes the return value using the original 'name' instead of the sanitized 'fname', which defeats the fallback logic and may produce incorrect filenames when 'name' is null or empty.

private string BuildFileName(string? name, string? extension, string defaultName, string defaultExtension)
{
    var fname = name.IfNullOrEmptyAs(defaultName);
    var fextension = extension.IfNullOrEmptyAs(defaultExtension);
    fextension = fextension.StartsWith(".") ? fextension.Substring(1) : fextension;
    return $"{name}.{fextension}";
}
Null Handling

GetImageConverter returns null but ConvertPdfToImages now returns the original PDF files in that case; ensure downstream consumers can handle PDFs when images are expected, or clarify intent to avoid unexpected behavior changes.

private async Task<IEnumerable<string>> ConvertPdfToImages(IImageConverter converter, IEnumerable<string> files)
{
    var images = new List<string>();
    if (converter == null || files.IsNullOrEmpty())
    {
        return files;
    }

    foreach (var file in files)
    {
Data Persistence

Saving intermediate assistant messages to DB is gated by 'SaveDataToDb'; verify deduplication and ordering so temporary messages don't interleave or duplicate with final messages in conversation history.

case ChatEvent.OnIntermediateMessageReceivedFromAssistant:
    if (!AllowSendingMessage()) return;

    model = new ChatResponseDto
    {
        ConversationId = conv.ConversationId,
        MessageId = message.MessageId,
        MessageLabel = message.MessageLabel,
        Text = !string.IsNullOrEmpty(message.SecondaryContent) ? message.SecondaryContent : message.Content,
        Function = message.FunctionName,
        RichContent = message.SecondaryRichContent ?? message.RichContent,
        Data = message.Data,
        Sender = new()
        {
            FirstName = "AI",
            LastName = "Assistant",
            Role = AgentRole.Assistant
        }
    };

    if (value.SaveDataToDb)
    {
        var storage = _services.GetRequiredService<IConversationStorage>();
        storage.Append(conv.ConversationId, message);
    }
    break;

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null check for database settings

Add a null check for _settings.Database before accessing its ConnectionString
property in InitializeDatabase to prevent a NullReferenceException and throw a
clear exception if the configuration is missing.

src/Plugins/BotSharp.Plugin.ExcelHandler/Services/MySqlService.cs [253-258]

 private void InitializeDatabase()
 {
-    _mysqlConnection = _settings.Database.ConnectionString;
+    _mysqlConnection = _settings.Database?.ConnectionString;
+    if (string.IsNullOrEmpty(_mysqlConnection))
+    {
+        throw new InvalidOperationException("MySQL connection string is not configured for ExcelHandler.");
+    }
     var databaseName = GetDatabaseName(_mysqlConnection);
     _logger.LogInformation($"Connected to MySQL database {databaseName}");
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential NullReferenceException if the Database configuration is missing and proposes a robust fix by adding a null check and throwing an informative exception.

Medium
Learned
best practice
Add null-safe defaults for args

Guard against null deserialization and null City by using the null-coalescing
operator to supply safe defaults.

src/Infrastructure/BotSharp.Core/Demo/Functions/GetWeatherFn.cs [23-50]

-var args = JsonSerializer.Deserialize<WeatherLocation>(message.FunctionArgs);
+var args = JsonSerializer.Deserialize<WeatherLocation>(message.FunctionArgs) ?? new WeatherLocation();
+var city = args.City ?? string.Empty;
 ...
-message.Indication = $"Start querying weather data in {args?.City}";
+message.Indication = $"Start querying weather data in {city}";
 ...
-var temp = RoleDialogModel.From(message, AgentRole.Assistant, $"Here is your weather in {args?.City}");
+var temp = RoleDialogModel.From(message, AgentRole.Assistant, $"Here is your weather in {city}");
 ...
-message.Indication = $"Still working on it... Hold on, {args?.City}";
+message.Indication = $"Still working on it... Hold on, {city}";

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Ensure null-safety on optional inputs and object properties to avoid NullReferenceExceptions.

Low
  • More

@iceljc iceljc merged commit b09d294 into SciSharp:master Sep 18, 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