Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Sep 17, 2025

PR Type

Bug fix, Enhancement


Description

  • Fix dialog retrieval using routing context

  • Improve file filtering with source validation

  • Enhance role checking with IsFromUser property

  • Clean up file references after processing


Diagram Walkthrough

flowchart LR
  A["Dialog Retrieval"] --> B["Routing Context Check"]
  B --> C["File Assembly"]
  C --> D["Source Validation"]
  D --> E["Role-based Filtering"]
Loading

File Walkthrough

Relevant files
Bug fix
HandleAudioRequestFn.cs
Integrate routing context for audio handling                         

src/Plugins/BotSharp.Plugin.AudioHandler/Functions/HandleAudioRequestFn.cs

  • Add routing context dependency injection
  • Implement fallback dialog retrieval logic
  • Enhance file filtering with source and role validation
  • Improve audio file assembly process
+17/-6   
HandleExcelRequestFn.cs
Enhance excel handler with routing integration                     

src/Plugins/BotSharp.Plugin.ExcelHandler/Functions/HandleExcelRequestFn.cs

  • Add routing context and state service dependencies
  • Implement dialog retrieval with routing context fallback
  • Refactor file assembly with improved filtering
  • Clean up file references after processing
+37/-16 
Enhancement
ReadImageFn.cs
Modernize role checking in image handler                                 

src/Plugins/BotSharp.Plugin.FileHandler/Functions/ReadImageFn.cs

  • Replace role-based checks with IsFromUser property
  • Update assistant role validation to IsFromAssistant
+2/-2     

@qodo-merge-pro
Copy link

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 Handling

After building the response, dialogs/files are not cleared like in the Excel handler. If large file lists are attached, consider clearing or avoiding mutation to prevent memory pressure or side effects in later pipeline stages.

var dialogs = AssembleFiles(conv.ConversationId, wholeDialogs);
var response = await GetResponeFromDialogs(dialogs);
message.Content = response;
return true;
Dialog Mutation

The code nulls dialog.Files after processing; validate that downstream components do not rely on Files for logging, auditing, or further processing in the same request lifecycle.

message.Content = GenerateSqlExecutionSummary(resultList);
states.SetState("excel_import_result",message.Content);
dialogs.ForEach(x => x.Files = null);
return true;
ContentType Check

Filtering uses string Contains("audio"); ensure ContentType is normalized and not null, or prefer exact/starts-with checks against known audio MIME types to avoid false positives.

audioFiles = audioFiles.Where(x => x.ContentType.Contains("audio")).ToList();

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Sep 17, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Add null-safety guards

Guard against nulls returned from deserialization and routing context retrieval,
and use safe defaults to avoid NullReferenceExceptions.

src/Plugins/BotSharp.Plugin.AudioHandler/Functions/HandleAudioRequestFn.cs [37-47]

-var args = JsonSerializer.Deserialize<LlmContextIn>(message.FunctionArgs, _options.JsonSerializerOptions);
+var args = JsonSerializer.Deserialize<LlmContextIn>(message.FunctionArgs, _options.JsonSerializerOptions) ?? new LlmContextIn();
 var conv = _serviceProvider.GetRequiredService<IConversationService>();
 var routingCtx = _serviceProvider.GetRequiredService<IRoutingContext>();
 
-var wholeDialogs = routingCtx.GetDialogs();
+var wholeDialogs = routingCtx?.GetDialogs() ?? new List<RoleDialogModel>();
 if (wholeDialogs.IsNullOrEmpty())
 {
-    wholeDialogs = conv.GetDialogHistory();
+    wholeDialogs = conv.GetDialogHistory() ?? new List<RoleDialogModel>();
 }
 
-var dialogs = AssembleFiles(conv.ConversationId, wholeDialogs);
+var dialogs = AssembleFiles(conv.ConversationId ?? string.Empty, wholeDialogs);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

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

Low
Apply comprehensive null checks

Add null checks and safe defaults for deserialization results, routing context
dialogs, conversation ID, and before mutating dialog files.

src/Plugins/BotSharp.Plugin.ExcelHandler/Functions/HandleExcelRequestFn.cs [51-77]

-var args = JsonSerializer.Deserialize<LlmContextIn>(message.FunctionArgs, _options.JsonSerializerOptions);
+var args = JsonSerializer.Deserialize<LlmContextIn>(message.FunctionArgs, _options.JsonSerializerOptions) ?? new LlmContextIn();
 var conv = _serviceProvider.GetRequiredService<IConversationService>();
 var states = _serviceProvider.GetRequiredService<IConversationStateService>();
 var routingCtx = _serviceProvider.GetRequiredService<IRoutingContext>();
 
 if (_excelMimeTypes.IsNullOrEmpty())
 {
     _excelMimeTypes = FileUtility.GetMimeFileTypes(new List<string> { "excel", "spreadsheet" }).ToHashSet<string>();
 }
 
-var dialogs = routingCtx.GetDialogs();
+var dialogs = routingCtx?.GetDialogs() ?? new List<RoleDialogModel>();
 if (dialogs.IsNullOrEmpty())
 {
-    dialogs = conv.GetDialogHistory();
+    dialogs = conv.GetDialogHistory() ?? new List<RoleDialogModel>();
 }
 
-var isExcelExist = AssembleFiles(conv.ConversationId, dialogs);
+var isExcelExist = AssembleFiles(conv.ConversationId ?? string.Empty, dialogs);
 if (!isExcelExist)
 {
     message.Content = "No excel files found in the conversation";
     return true;
 }
 
 var resultList = GetResponeFromDialogs(dialogs);
 message.Content = GenerateSqlExecutionSummary(resultList);
-states.SetState("excel_import_result",message.Content);
-dialogs.ForEach(x => x.Files = null);
+states.SetState("excel_import_result", message.Content);
+dialogs?.ForEach(x => x.Files = null);
  • Apply / Chat
Suggestion importance[1-10]: 5

__

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

Low
High-level
Simplify redundant file source filtering

Remove the redundant FileSource.User check inside the loop in the AssembleFiles
method for both audio and excel handlers. The files are already filtered by
source in the initial query.

Examples:

src/Plugins/BotSharp.Plugin.AudioHandler/Functions/HandleAudioRequestFn.cs [71-72]
            var found = audioFiles.Where(x => x.MessageId == dialog.MessageId
                                           && x.FileSource.IsEqualTo(FileSource.User)).ToList();
src/Plugins/BotSharp.Plugin.ExcelHandler/Functions/HandleExcelRequestFn.cs [105-106]
            var found = excelFiles.Where(x => x.MessageId == dialog.MessageId
                                           && x.FileSource.IsEqualTo(FileSource.User)).ToList();

Solution Walkthrough:

Before:

private List<RoleDialogModel> AssembleFiles(...)
{
    var audioFiles = _fileStorage.GetMessageFiles(..., options: new()
    {
        Sources = [FileSource.User],
        ...
    });

    foreach (var dialog in dialogs)
    {
        var found = audioFiles.Where(x => x.MessageId == dialog.MessageId
                                       && x.FileSource.IsEqualTo(FileSource.User)).ToList(); // Redundant check
        ...
    }
    return dialogs;
}

After:

private List<RoleDialogModel> AssembleFiles(...)
{
    var audioFiles = _fileStorage.GetMessageFiles(..., options: new()
    {
        Sources = [FileSource.User],
        ...
    });

    foreach (var dialog in dialogs)
    {
        var found = audioFiles.Where(x => x.MessageId == dialog.MessageId).ToList(); // Redundant check removed
        ...
    }
    return dialogs;
}
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a logical redundancy in two separate files, and removing it would simplify the code, but the impact on performance and overall functionality is minor.

Low
General
Remove redundant file source check

Remove the redundant x.FileSource.IsEqualTo(FileSource.User) check from the
Where clause, as the audioFiles collection is already filtered by user source.

src/Plugins/BotSharp.Plugin.AudioHandler/Functions/HandleAudioRequestFn.cs [71-72]

-var found = audioFiles.Where(x => x.MessageId == dialog.MessageId
-                                       && x.FileSource.IsEqualTo(FileSource.User)).ToList();
+var found = audioFiles.Where(x => x.MessageId == dialog.MessageId).ToList();
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a redundant condition, as the audioFiles collection is already filtered by FileSource.User upon retrieval. Removing this check improves code clarity.

Low
  • Update

@iceljc iceljc merged commit c391051 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