Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Sep 9, 2025

PR Type

Enhancement


Description

  • Add message wrapper support to chat responses

  • Implement ChatResponseWrapper class with sending interval

  • Map additional message wrapper from conversation messages

  • Include message label field in response model


Diagram Walkthrough

flowchart LR
  A["ConversationController"] --> B["ChatResponseModel"]
  B --> C["ChatResponseWrapper"]
  C --> D["Additional Messages"]
  A --> E["Message Label Mapping"]
Loading

File Walkthrough

Relevant files
Enhancement
ConversationController.cs
Map message wrapper and label fields                                         

src/Infrastructure/BotSharp.OpenAPI/Controllers/ConversationController.cs

  • Add MessageLabel mapping from message to response
  • Map AdditionalMessageWrapper using new wrapper class
  • Apply changes to both sync and async message handlers
+4/-0     
ChatResponseModel.cs
Implement chat response wrapper classes                                   

src/Infrastructure/BotSharp.OpenAPI/ViewModels/Conversations/Response/ChatResponseModel.cs

  • Add AdditionalMessageWrapper property with JSON serialization
  • Implement ChatResponseWrapper class with sending interval
  • Create static From method for wrapper conversion
  • Map conversation messages to response models
+39/-0   

@iceljc iceljc merged commit 89e1acf into SciSharp:master Sep 9, 2025
0 of 4 checks passed
Copy link

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

Null Handling

Ensure msg.AdditionalMessageWrapper can be null without causing issues when passed to ChatResponseWrapper.From; also confirm inputMsg.MessageId is available and non-null when used.

await conv.SendMessage(agentId, inputMsg,
    replyMessage: input.Postback,
    async msg =>
    {
        response.Text = !string.IsNullOrEmpty(msg.SecondaryContent) ? msg.SecondaryContent : msg.Content;
        response.Function = msg.FunctionName;
        response.MessageLabel = msg.MessageLabel;
        response.RichContent = msg.SecondaryRichContent ?? msg.RichContent;
        response.Instruction = msg.Instruction;
        response.Data = msg.Data;
        response.AdditionalMessageWrapper = ChatResponseWrapper.From(msg.AdditionalMessageWrapper, conversationId, inputMsg.MessageId);
    });
MessageId Mapping

The wrapper maps MessageId = messageId ?? x.MessageId; verify this logic matches client expectations and does not overwrite distinct child message IDs unintentionally.

    public static ChatResponseWrapper? From(ChatMessageWrapper? wrapper, string conversationId, string? messageId = null)
    {
        if (wrapper == null)
        {
            return null;
        }

        return new ChatResponseWrapper
        {
            SendingInterval = wrapper.SendingInterval,
            Messages = wrapper?.Messages?.Select(x => new ChatResponseModel
            {
                ConversationId = conversationId,
                MessageId = messageId ?? x.MessageId,
                Text = !string.IsNullOrEmpty(x.SecondaryContent) ? x.SecondaryContent : x.Content,
                MessageLabel = x.MessageLabel,
                Function = x.FunctionName,
                RichContent = x.SecondaryRichContent ?? x.RichContent,
                Instruction = x.Instruction,
                Data = x.Data,
                IsAppend = true
            })?.ToList()
        };
    }
}
Serialization Consistency

New JSON properties (additional_message_wrapper, sending_interval, messages) should align with API contract; verify casing and null-ignores across sync/stream responses for backward compatibility.

    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    [JsonPropertyName("additional_message_wrapper")]
    public ChatResponseWrapper? AdditionalMessageWrapper { get; set; }
}

public class ChatResponseWrapper
{
    [JsonPropertyName("sending_interval")]
    public int SendingInterval { get; set; }

    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    [JsonPropertyName("messages")]
    public List<ChatResponseModel>? Messages { get; set; }

Copy link

qodo-merge-pro bot commented Sep 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Preserve original message IDs

The wrapper conversion currently sets each additional message's MessageId to the
parent input message ID (MessageId = messageId ?? x.MessageId with a non-null
parent passed), causing duplicate IDs across different messages. This can break
client-side deduplication, SSE streaming reconciliation, and downstream
operations (update/delete/analytics) that assume per-message uniqueness. Use
x.MessageId whenever present and, if grouping is required, add a separate
parentMessageId or rely on the wrapper context rather than overriding child
message IDs.

Examples:

src/Infrastructure/BotSharp.OpenAPI/ViewModels/Conversations/Response/ChatResponseModel.cs [32-43]
            Messages = wrapper?.Messages?.Select(x => new ChatResponseModel
            {
                ConversationId = conversationId,
                MessageId = messageId ?? x.MessageId,
                Text = !string.IsNullOrEmpty(x.SecondaryContent) ? x.SecondaryContent : x.Content,
                MessageLabel = x.MessageLabel,
                Function = x.FunctionName,
                RichContent = x.SecondaryRichContent ?? x.RichContent,
                Instruction = x.Instruction,
                Data = x.Data,

 ... (clipped 2 lines)
src/Infrastructure/BotSharp.OpenAPI/Controllers/ConversationController.cs [378]
                response.AdditionalMessageWrapper = ChatResponseWrapper.From(msg.AdditionalMessageWrapper, conversationId, inputMsg.MessageId);

Solution Walkthrough:

Before:

// In ChatResponseWrapper.From(...)
Messages = wrapper?.Messages?.Select(x => new ChatResponseModel
{
    ConversationId = conversationId,
    // The parent messageId is incorrectly prioritized,
    // causing all wrapped messages to share the same ID.
    MessageId = messageId ?? x.MessageId,
    Text = x.Content,
    // ... other properties
})?.ToList()

After:

// In ChatResponseWrapper.From(...)
Messages = wrapper?.Messages?.Select(x => new ChatResponseModel
{
    ConversationId = conversationId,
    // The message's own ID is correctly preserved.
    MessageId = x.MessageId,
    // A new property could be added to link back to the parent message.
    ParentMessageId = messageId,
    Text = x.Content,
    // ... other properties
})?.ToList()
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical bug where all additional messages are assigned the same ID as the parent message, which would break fundamental client-side and backend operations that rely on unique message IDs.

High
Possible issue
Reference assistant message ID

Bind additional messages to the assistant's reply, not the user's input. Use
msg.MessageId so appended messages share the correct parent ID and don't get
misassociated with the user's message. This prevents threading issues and
accidental overwrites.

src/Infrastructure/BotSharp.OpenAPI/Controllers/ConversationController.cs [378]

-response.AdditionalMessageWrapper = ChatResponseWrapper.From(msg.AdditionalMessageWrapper, conversationId, inputMsg.MessageId);
+response.AdditionalMessageWrapper = ChatResponseWrapper.From(msg.AdditionalMessageWrapper, conversationId, msg.MessageId);
 ...
-response.AdditionalMessageWrapper = ChatResponseWrapper.From(msg.AdditionalMessageWrapper, conversationId, inputMsg.MessageId);
+response.AdditionalMessageWrapper = ChatResponseWrapper.From(msg.AdditionalMessageWrapper, conversationId, msg.MessageId);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that additional messages, being part of the assistant's response (msg), should be associated with the assistant's message ID (msg.MessageId) instead of the user's input message ID (inputMsg.MessageId), which is crucial for maintaining correct conversation threading.

Medium
  • More

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