Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Sep 6, 2025

PR Type

Enhancement


Description

  • Add indication support for chat messages

  • Refactor sender object creation for reusability

  • Enable indication events in chat hub


Diagram Walkthrough

flowchart LR
  A["PlotChartFn"] -- "adds indication" --> B["RoleDialogModel"]
  C["ChatHubConversationHook"] -- "creates reusable" --> D["UserDto sender"]
  C -- "sends indication event" --> E["OnIndicationReceived"]
  B -- "flows to" --> C
Loading

File Walkthrough

Relevant files
Enhancement
PlotChartFn.cs
Add summarizing indication to chart function                         

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

  • Add Indication = "Summarizing" property to RoleDialogModel message
+1/-0     
ChatHubConversationHook.cs
Add indication support and refactor sender creation           

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

  • Create reusable UserDto sender object for AI Assistant
  • Add indication event handling with OnIndicationReceived
  • Replace inline sender creation with reusable sender variable
  • Check for indication in wrapper messages and send indication events
+22/-12 

Copy link

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

Event Ordering

When both indication and text are present for wrapper messages, ensure the sequencing of OnIndicationReceived followed by OnMessageReceivedFromAssistant (with delays) matches client expectations and doesn’t cause UI flicker or race conditions, especially with streaming enabled.

    if (!string.IsNullOrWhiteSpace(item.Indication))
    {
        data = new ChatResponseDto
        {
            ConversationId = conv.ConversationId,
            MessageId = item.MessageId,
            Indication = item.Indication,
            Sender = sender
        };
        await SendEvent(ChatEvent.OnIndicationReceived, conv.ConversationId, data);
    }

    await Task.Delay(wrapper.SendingInterval);

    data = new ChatResponseDto
    {
        ConversationId = conv.ConversationId,
        MessageId = item.MessageId,
        Text = !string.IsNullOrEmpty(item.SecondaryContent) ? item.SecondaryContent : item.Content,
        Function = item.FunctionName,
        RichContent = item.SecondaryRichContent ?? item.RichContent,
        Data = item.Data,
        States = state.GetStates(),
        IsAppend = true,
        Sender = sender
    };
    await SendEvent(ChatEvent.OnMessageReceivedFromAssistant, conv.ConversationId, data);
}
State Consistency

Indication events send a ChatResponseDto without States or other contextual fields; verify clients don’t rely on these fields for rendering or correlation, or include minimal required context.

    data = new ChatResponseDto
    {
        ConversationId = conv.ConversationId,
        MessageId = item.MessageId,
        Indication = item.Indication,
        Sender = sender
    };
    await SendEvent(ChatEvent.OnIndicationReceived, conv.ConversationId, data);
}
Indication Value

The hard-coded indication string "Summarizing" should align with a defined enum or constants to avoid typos and enable i18n; confirm consumers expect this exact value.

Indication = "Summarizing",
FunctionName = message.FunctionName,

Copy link

qodo-merge-pro bot commented Sep 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Handle indication lifecycle globally

Indication events are only emitted for wrapper messages; a top-level message’s
Indication is ignored and there’s no explicit “end” signal, risking stale UI
state. Emit OnIndicationReceived when message.Indication is set on the primary
message as well, and send a clear/termination signal (e.g., empty indication or
a dedicated OnIndicationCleared) after final content or on error to ensure
consistent client behavior.

Examples:

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubConversationHook.cs [92-170]
    public override async Task OnResponseGenerated(RoleDialogModel message)
    {
        if (!AllowSendingMessage()) return;

        var conv = _services.GetRequiredService<IConversationService>();
        var state = _services.GetRequiredService<IConversationStateService>();

        var sender = new UserDto
        {
            FirstName = "AI",

 ... (clipped 69 lines)

Solution Walkthrough:

Before:

public override async Task OnResponseGenerated(RoleDialogModel message)
{
    // ... initial response is sent, but message.Indication is ignored.

    var wrapper = message.AdditionalMessageWrapper;
    if (wrapper?.Messages?.Count > 0)
    {
        foreach (var item in wrapper.Messages)
        {
            if (!string.IsNullOrWhiteSpace(item.Indication))
            {
                // Send OnIndicationReceived event for wrapped message
            }

            // ... delay ...

            // Send message content.
            // Client must infer that this clears the indication.
        }
    }
}

After:

public override async Task OnResponseGenerated(RoleDialogModel message)
{
    if (!string.IsNullOrWhiteSpace(message.Indication))
    {
        // Send OnIndicationReceived for top-level message
    }

    // ... send initial response ...
    // Send OnIndicationCleared event (or empty indication)

    var wrapper = message.AdditionalMessageWrapper;
    if (wrapper?.Messages?.Count > 0)
    {
        foreach (var item in wrapper.Messages)
        {
            if (!string.IsNullOrWhiteSpace(item.Indication))
            {
                // Send OnIndicationReceived event
            }
            // ... delay and send message content ...
            // Send OnIndicationCleared event
        }
    }
}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a design flaw where the indication lifecycle is incomplete, as it only handles wrapped messages and lacks an explicit clearing mechanism, which could lead to UI state bugs.

Medium
Possible issue
Emit top-level indication event

If the top-level message carries an Indication, it is never emitted (only
wrapper items are handled). Emit an indication event for the base message so
clients receive it even when no wrapper is used. Send it before the normal
assistant message event to preserve ordering.

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubConversationHook.cs [119-126]

 // Send type-off to client
 var action = new ConversationSenderActionModel
 {
     ConversationId = conv.ConversationId,
     SenderAction = SenderActionEnum.TypingOff
 };
 await SendEvent(ChatEvent.OnSenderActionGenerated, conv.ConversationId, action);
+
+if (!string.IsNullOrWhiteSpace(message.Indication))
+{
+    var indicationData = new ChatResponseDto
+    {
+        ConversationId = conv.ConversationId,
+        MessageId = message.MessageId,
+        Indication = message.Indication,
+        Sender = sender
+    };
+    await SendEvent(ChatEvent.OnIndicationReceived, conv.ConversationId, indicationData);
+}
+
 await SendEvent(ChatEvent.OnMessageReceivedFromAssistant, conv.ConversationId, data);
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the new Indication feature is only implemented for wrapped messages, and not for the main message, which is an oversight that makes the feature incomplete.

Medium
General
Skip empty wrapper messages

Avoid sending empty assistant messages when a wrapper item only contains an
indication and no content. Check for any text, rich content, or data before
emitting the message event, and skip if none is present.

src/Plugins/BotSharp.Plugin.ChatHub/Hooks/ChatHubConversationHook.cs [148-162]

 await Task.Delay(wrapper.SendingInterval);
 
-data = new ChatResponseDto
+var text = !string.IsNullOrEmpty(item.SecondaryContent) ? item.SecondaryContent : item.Content;
+var rich = item.SecondaryRichContent ?? item.RichContent;
+var hasPayload = !(string.IsNullOrWhiteSpace(text) && rich == null && item.Data == null);
+if (hasPayload)
 {
-    ConversationId = conv.ConversationId,
-    MessageId = item.MessageId,
-    Text = !string.IsNullOrEmpty(item.SecondaryContent) ? item.SecondaryContent : item.Content,
-    Function = item.FunctionName,
-    RichContent = item.SecondaryRichContent ?? item.RichContent,
-    Data = item.Data,
-    States = state.GetStates(),
-    IsAppend = true,
-    Sender = sender
-};
-await SendEvent(ChatEvent.OnMessageReceivedFromAssistant, conv.ConversationId, data);
+    data = new ChatResponseDto
+    {
+        ConversationId = conv.ConversationId,
+        MessageId = item.MessageId,
+        Text = text,
+        Function = item.FunctionName,
+        RichContent = rich,
+        Data = item.Data,
+        States = state.GetStates(),
+        IsAppend = true,
+        Sender = sender
+    };
+    await SendEvent(ChatEvent.OnMessageReceivedFromAssistant, conv.ConversationId, data);
+}
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out a potential issue where a message containing only an Indication would also trigger an empty assistant message event, which is likely unintended and could cause issues on the client side.

Medium
  • More

@iceljc iceljc merged commit 9fb6d9f into SciSharp:master Sep 6, 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