Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Sep 17, 2025

PR Type

Enhancement


Description

  • Add audio transcription settings to LLM model configuration

  • Refactor audio handler with improved provider/model selection

  • Restructure excel handler with unified database service interface

  • Remove unused interfaces and consolidate database implementations


Diagram Walkthrough

flowchart LR
  A["LlmModelSetting"] --> B["AudioSetting"]
  C["HandleAudioRequestFn"] --> D["ReadAudioFn"]
  E["Multiple DB Interfaces"] --> F["Unified IDbService"]
  G["MySqlService"] --> H["Enhanced Implementation"]
  I["SqliteService"] --> J["Streamlined Code"]
Loading

File Walkthrough

Relevant files
Enhancement
20 files
LlmModelSetting.cs
Add audio transcription settings structure                             
+19/-0   
HandleAudioRequestFn.cs
Remove old audio handler function                                               
+0/-135 
ReadAudioFn.cs
Add new audio reading function                                                     
+171/-0 
AudioHandlerSettings.cs
Expand audio handler settings structure                                   
+14/-0   
Using.cs
Add missing global using statements                                           
+4/-0     
ExcelHandlerPlugin.cs
Simplify dependency injection registration                             
+2/-6     
ReadExcelFn.cs
Refactor excel reading with unified service                           
+57/-61 
IMySqlDbHelper.cs
Remove MySQL helper interface                                                       
+0/-15   
MySqlDbHelpers.cs
Remove MySQL helper implementation                                             
+0/-44   
ISqliteDbHelpers.cs
Remove SQLite helper interface                                                     
+0/-9     
SqliteDbHelpers.cs
Remove SQLite helper implementation                                           
+0/-41   
IDbService.cs
Simplify database service interface                                           
+6/-11   
IMySqlService.cs
Remove MySQL specific service interface                                   
+0/-6     
ISqliteService.cs
Remove SQLite specific service interface                                 
+0/-16   
MySqlService.cs
Refactor MySQL service with direct connection                       
+215/-207
SqliteService.cs
Refactor SQLite service with direct connection                     
+218/-193
ExcelHandlerSettings.cs
Add database provider configuration                                           
+1/-6     
Using.cs
Add missing global using statements                                           
+6/-0     
FileHandlerSettings.cs
Remove unused image variation settings                                     
+0/-6     
AudioTranscriptionProvider.cs
Enhance transcription options with model settings               
+39/-7   
Configuration changes
3 files
BotSharp.sln
Add ExcelHandler project to solution                                         
+11/-0   
WebStarter.csproj
Add ExcelHandler project reference                                             
+1/-0     
appsettings.json
Add audio and excel handler configurations                             
+29/-18 

@qodo-merge-pro
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

Conversation state keys for provider/model are both fetched from audio_read_llm_provider; likely the model should use a different key (e.g., audio_read_llm_model). This will cause the model to mirror the provider value.

    var provider = state.GetState("audio_read_llm_provider");
    var model = state.GetState("audio_read_llm_provider");

    if (!string.IsNullOrEmpty(provider) && !string.IsNullOrEmpty(model))
    {
        return (provider, model);
    }

    provider = _settings?.Audio?.Reading?.LlmProvider;
    model = _settings?.Audio?.Reading?.LlmModel;

    if (!string.IsNullOrEmpty(provider) && !string.IsNullOrEmpty(model))
    {
        return (provider, model);
    }

    provider = "openai";
    model = "gpt-4o-mini-transcribe";

    return (provider, model);
}
DI Conflict

Registering two implementations for the same IDbService without resolution logic can cause ambiguity. Consider named registrations or selecting the default provider via configuration to avoid unintended service resolution.

    services.AddScoped<IAgentUtilityHook, ExcelHandlerUtilityHook>();
    services.AddScoped<IDbService, SqliteService>();
    services.AddScoped<IDbService, MySqlService>();
}
Content-Type Check

VerifyAudioFileType accepts files if either extension matches enum or content type is non-empty, which may pass unsupported audio types. Tighten validation to ensure only supported audio formats proceed.

private bool VerifyAudioFileType(string fileName)
{
    var extension = Path.GetExtension(fileName).TrimStart('.').ToLower();
    return Enum.TryParse<AudioType>(extension, out _)
                || !string.IsNullOrEmpty(FileUtility.GetFileContentType(fileName));
}

@qodo-merge-pro
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix copy-paste error in state key

Correct the state key used for retrieving the LLM model name from
"audio_read_llm_provider" to "audio_read_llm_model" to fix a copy-paste error.

src/Plugins/BotSharp.Plugin.AudioHandler/Functions/ReadAudioFn.cs [150-151]

 var provider = state.GetState("audio_read_llm_provider");
-var model = state.GetState("audio_read_llm_provider");
+var model = state.GetState("audio_read_llm_model");
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a copy-paste bug where the same state key is used to fetch both the provider and the model, which would lead to incorrect model selection.

Medium
Prevent unhandled exception when no files

Replace Last() with LastOrDefault() and add a null check for the dialog object
to prevent a potential InvalidOperationException when no dialog with files is
found.

src/Plugins/BotSharp.Plugin.ExcelHandler/Functions/ReadExcelFn.cs [118-128]

 private List<SqlContextOut> GetResponeFromDialogs(List<RoleDialogModel> dialogs)
 {
     var sqlCommands = new List<SqlContextOut>();
-    var dialog = dialogs.Last(x => !x.Files.IsNullOrEmpty());
+    var dialog = dialogs.LastOrDefault(x => !x.Files.IsNullOrEmpty());
+    if (dialog?.Files == null)
+    {
+        return sqlCommands;
+    }
     
     foreach (var file in dialog.Files)
     {
         if (string.IsNullOrWhiteSpace(file?.FileStorageUrl))
         {
             continue;
         }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that using Last() can throw an InvalidOperationException if no item matches the predicate. Replacing it with LastOrDefault() and adding a null check makes the code more robust.

Medium
High-level
Unify database services using a factory

Instead of injecting all IDbService implementations and filtering in the
ReadExcelFn constructor, use a factory pattern. This centralizes the logic for
selecting the correct database service based on configuration.

Examples:

src/Plugins/BotSharp.Plugin.ExcelHandler/Functions/ReadExcelFn.cs [22-36]
    public ReadExcelFn(
        IServiceProvider services,
        ILogger<ReadExcelFn> logger,
        BotSharpOptions options,
        ExcelHandlerSettings settings,
        IFileStorageService fileStorage,
        IEnumerable<IDbService> dbServices)
    {
        _services = services;
        _logger = logger;

 ... (clipped 5 lines)
src/Plugins/BotSharp.Plugin.ExcelHandler/ExcelHandlerPlugin.cs [26-27]
        services.AddScoped<IDbService, SqliteService>();
        services.AddScoped<IDbService, MySqlService>();

Solution Walkthrough:

Before:

// In ExcelHandlerPlugin.cs
services.AddScoped<IDbService, SqliteService>();
services.AddScoped<IDbService, MySqlService>();

// In ReadExcelFn.cs
public class ReadExcelFn
{
    private readonly IDbService _dbService;

    public ReadExcelFn(IEnumerable<IDbService> dbServices, ExcelHandlerSettings settings, ...)
    {
        _dbService = dbServices.FirstOrDefault(x => x.Provider == settings.DbProvider);
        // ...
    }
    // ...
}

After:

// In ExcelHandlerPlugin.cs
services.AddScoped<SqliteService>();
services.AddScoped<MySqlService>();
services.AddScoped<IDbService>(sp => {
    var settings = sp.GetRequiredService<ExcelHandlerSettings>();
    if (settings.DbProvider == "mysql") {
        return sp.GetRequiredService<MySqlService>();
    } else { // "sqlite"
        return sp.GetRequiredService<SqliteService>();
    }
});

// In ReadExcelFn.cs
public class ReadExcelFn
{
    private readonly IDbService _dbService;

    public ReadExcelFn(IDbService dbService, ...)
    {
        _dbService = dbService;
        // ...
    }
    // ...
}
Suggestion importance[1-10]: 6

__

Why: The suggestion provides a valid architectural improvement by proposing a factory pattern to centralize database service selection, which is cleaner than injecting an IEnumerable and filtering in the consumer.

Low
Learned
best practice
Add null checks to collections

Guard against null dialog.Files before iterating and coalesce potentially null
strings to safe defaults to avoid NullReferenceExceptions.

src/Plugins/BotSharp.Plugin.AudioHandler/Functions/ReadAudioFn.cs [91-101]

 var dialog = dialogs.Where(x => !x.Files.IsNullOrEmpty()).LastOrDefault();
 var transcripts = new List<string>();
 
-if (dialog != null)
+if (dialog?.Files != null && dialog.Files.Count > 0)
 {
     foreach (var file in dialog.Files)
     {
-        if (string.IsNullOrWhiteSpace(file?.FileStorageUrl))
+        if (string.IsNullOrWhiteSpace(file?.FileStorageUrl ?? string.Empty))
         {
             continue;
         }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

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

Low
General
Remove redundant stream close call

Remove the redundant stream.Close() call inside the using block, as the using
statement already ensures the stream is properly disposed.

src/Plugins/BotSharp.Plugin.AudioHandler/Functions/ReadAudioFn.cs [110-117]

 var binary = _fileStorage.GetFileBytes(file.FileStorageUrl);
 using var stream = binary.ToStream();
 stream.Position = 0;
 
 var result = await audioCompletion.TranscriptTextAsync(stream, fileName);
 transcripts.Add(result);
-stream.Close();
 await Task.Delay(100);
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that calling stream.Close() is redundant within a using block, which improves code style and adheres to best practices for resource management.

Low
  • More

@iceljc iceljc merged commit 22ce231 into SciSharp:master Sep 17, 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