Skip to content

refine naming #1148

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 5, 2025
Merged

refine naming #1148

iceljc merged 1 commit into from
Sep 5, 2025

Conversation

@iceljc
Copy link
Collaborator

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

PR Type

Enhancement

Description

  • Renamed IntervalMilliSeconds to SendingInterval for clarity

  • Added comprehensive XML documentation to ChatMessageWrapper properties

  • Improved conditional logic for message wrapper validation

Diagram Walkthrough

flowchart LR
  A["ChatMessageWrapper"] --> B["Rename IntervalMilliSeconds"]
  B --> C["Add XML Documentation"]
  C --> D["Update Usage References"]
Loading

File Walkthrough
Relevant files
Enhancement
RoleDialogModel.cs
Enhance ChatMessageWrapper with better naming and docs     

src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs

  • Renamed IntervalMilliSeconds property to SendingInterval
  • Added XML documentation for all three properties
  • Improved code readability with descriptive comments
 +12/-1   
PlotChartFn.cs
Update property reference to new name                                       

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

  • Updated property reference from IntervalMilliSeconds to
    SendingInterval
 +1/-1     
ChatHubConversationHook.cs
Refactor message wrapper usage with improved validation   

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubConversationHook.cs

  • Updated property reference from IntervalMilliSeconds to
    SendingInterval
  • Added local variable wrapper for cleaner code
  • Enhanced conditional check to validate SendingInterval > 0
 +4/-3     

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

qodo-code-review bot commented Sep 5, 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

Behavior Change

The previous default of 1000 ms for the interval was removed; the new SendingInterval has no default. Verify that all callers set a positive value or that downstream logic handles null/zero appropriately to avoid tight loops or no delays.

 
/// <summary>
/// Messages sending interval in milliseconds
/// </summary>
public int SendingInterval { get; set; }

/// <summary>
/// Whether the Messages are saved to db
/// </summary>
public bool SaveToDb { get; set; }

/// <summary>
/// Messages to send or save
/// </summary>
public List<RoleDialogModel>? Messages { get; set; }
Null-Conditional Logic

The condition uses wrapper?.SendingInterval > 0 and wrapper?.Messages?.Count > 0. While safe, ensure that when wrapper is null or interval <= 0, the intended fallback behavior (no typing-on and no additional messages) is correct and does not regress user experience.

 
var wrapper = message.AdditionalMessageWrapper;
if (wrapper?.SendingInterval > 0 && wrapper?.Messages?.Count > 0)
{
    action.SenderAction = SenderActionEnum.TypingOn;
    await SendEvent(ChatEvent.OnSenderActionGenerated, conv.ConversationId, action);

    foreach (var item in wrapper.Messages)
    {
        await Task.Delay(wrapper.SendingInterval);

@qodo-code-review
Copy link

qodo-code-review bot commented Sep 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add safe default interval

Without a default value this becomes 0, which stops additional messages from
being sent under the new check. Set a sensible default to preserve prior
behavior and prevent silent skips.

src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs [189-192]

 /// <summary>
 /// Messages sending interval in milliseconds
 /// </summary>
-public int SendingInterval { get; set; }
+public int SendingInterval { get; set; } = 1000;
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies that removing the default value from SendingInterval introduces a behavioral regression, and proposes restoring the default value, which is a direct and effective fix for the issue.

High
Fix nullable check and default delay

The null-conditional comparison returns a nullable bool, causing a compile error
in the if condition. Also, if SendingInterval is unset (0), additional messages
will never send. Coalesce counts and inject a safe default delay to ensure both
compilation and expected behavior.

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubConversationHook.cs [125-133]

 var wrapper = message.AdditionalMessageWrapper;
-if (wrapper?.SendingInterval > 0 && wrapper?.Messages?.Count > 0)
+if ((wrapper?.Messages?.Count ?? 0) > 0)
 {
     action.SenderAction = SenderActionEnum.TypingOn;
     await SendEvent(ChatEvent.OnSenderActionGenerated, conv.ConversationId, action);
 
-    foreach (var item in wrapper.Messages)
+    var delayMs = wrapper?.SendingInterval > 0 ? wrapper.SendingInterval : 1000;
+    foreach (var item in wrapper!.Messages)
     {
-        await Task.Delay(wrapper.SendingInterval);
+        await Task.Delay(delayMs);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that messages will not be sent if SendingInterval is 0 (its new default), and proposes a robust fix by using a default delay, thus preventing a silent regression introduced by the PR.

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