Skip to content

Test/realtime chat #1034

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.

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Test/realtime chat #1034

wants to merge 9 commits into from

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Apr 25, 2025

PR Type

enhancement


Description

  • Introduced ChatStreamMiddleware for real-time chat streaming via WebSockets

  • Added models for chat stream event deserialization

  • Refactored and improved real-time completion providers for OpenAI and Google AI

  • Registered new middleware in web application startup

  • Minor bug fixes and code cleanups in related files


Changes walkthrough 📝

Relevant files
Enhancement
15 files
ChatStreamMiddleware.cs
Add middleware for real-time chat streaming via WebSockets
+159/-0 
ChatStreamEventResponse.cs
Add models for chat stream event deserialization                 
+15/-0   
Program.cs
Register new chat streaming middleware in app startup       
+4/-1     
Using.cs
Add global using for chat stream models                                   
+2/-1     
ChatHubMiddleware.cs
Rename middleware class for chat hub                                         
+2/-2     
RealTimeCompletionProvider.cs
Refactor Google AI real-time provider for clarity and maintainability
+29/-27 
RealTimeCompletionProvider.cs
Refactor OpenAI real-time provider and improve session handling
+51/-27 
RealtimeChatSession.cs
Make chat session class internal and update type names     
+4/-4     
AsyncWebsocketDataCollectionResult.cs
Make async websocket data collection result class internal
+1/-1     
AsyncWebsocketDataResultEnumerator.cs
Make async websocket data result enumerator class internal
+1/-1     
AiWebsocketPipelineResponse.cs
Make pipeline response class internal                                       
+1/-2     
ChatSessionUpdate.cs
Rename session update class for clarity                                   
+2/-2     
RealtimeHub.cs
Refactor model connection call for clarity                             
+2/-1     
IRealTimeCompletion.cs
Refactor Connect method signature for readability               
+2/-1     
Program.cs
Increase buffer size for audio streaming                                 
+1/-1     
Bug fix
1 files
RoutingContext.cs
Remove unused parameter in agent options retrieval             
+1/-1     
Configuration changes
1 files
CrontabPlugin.cs
Comment out hosted service registration for crontab           
+2/-2     
Formatting
2 files
ConversationItemCreated.cs
Add newline and formatting for model class                             
+1/-0     
ConversationController.cs
Add newline at end of file                                                             
+1/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    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

    Error Handling

    The error handling in the WebSocket connection is minimal. The middleware catches exceptions but doesn't properly close the WebSocket connection in error scenarios, which could lead to resource leaks.

        try
        {
            var services = httpContext.RequestServices;
            var segments = request.Path.Value.Split("/");
            var agentId = segments[segments.Length - 2];
            var conversationId = segments[segments.Length - 1];
    
            using var webSocket = await httpContext.WebSockets.AcceptWebSocketAsync();
            await HandleWebSocket(services, agentId, conversationId, webSocket);
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, $"Error when connecting Chat stream. ({ex.Message})");
        }
        return;
    }
    Missing Initialization

    The OpenAI RealTimeCompletionProvider constructor is missing initialization for the _settings parameter, which could lead to null reference exceptions when the settings are accessed.

        RealtimeModelSettings settings,
        ILogger<RealTimeCompletionProvider> logger,
        IServiceProvider services,
        BotSharpOptions botsharpOptions)
    {
        _settings = settings;
        _logger = logger;
        _services = services;
        _botsharpOptions = botsharpOptions;
    }
    Parameter Change

    The GetAgentOptions method call was modified to remove the 'byName: true' parameter, which might change the behavior of agent lookup by name.

    var agents = agentService.GetAgentOptions([agentId]).Result;

    Copy link

    qodo-merge-pro bot commented Apr 25, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix WebSocket fragmentation handling

    The code is using a fixed-size buffer for receiving WebSocket messages, but it
    doesn't handle fragmented messages correctly. WebSocket messages can be split
    across multiple frames, and the current implementation will process each frame
    independently, potentially causing data corruption for large messages.

    src/Plugins/BotSharp.Plugin.ChatHub/ChatStreamMiddleware.cs [61-77]

     var buffer = new byte[1024 * 32];
     WebSocketReceiveResult result;
    +var messageBuffer = new List<byte>();
     
     do
     {
         result = await webSocket.ReceiveAsync(new(buffer), CancellationToken.None);
     
         if (result.MessageType != WebSocketMessageType.Text)
         {
             continue;
         }
     
    -    var receivedText = Encoding.UTF8.GetString(buffer, 0, result.Count);
    -    if (string.IsNullOrEmpty(receivedText))
    +    messageBuffer.AddRange(new ArraySegment<byte>(buffer, 0, result.Count));
    +    
    +    if (result.EndOfMessage)
         {
    -        continue;
    -    }
    +        var receivedText = Encoding.UTF8.GetString(messageBuffer.ToArray());
    +        messageBuffer.Clear();
    +        
    +        if (string.IsNullOrEmpty(receivedText))
    +        {
    +            continue;
    +        }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a significant issue with WebSocket message handling. The current implementation doesn't properly handle fragmented messages, which could lead to data corruption for large messages. The improved code properly accumulates message fragments before processing.

    Medium
    Check WebSocket state

    The SendEventToUser method doesn't check if the WebSocket is in a valid state
    before sending data. If the client has disconnected or the connection is
    closing, this could throw an exception and crash the middleware.

    src/Plugins/BotSharp.Plugin.ChatHub/ChatStreamMiddleware.cs [106]

    -await SendEventToUser(webSocket, data);
    +if (webSocket.State == WebSocketState.Open)
    +{
    +    await SendEventToUser(webSocket, data);
    +}

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: This suggestion addresses an important defensive programming practice by checking the WebSocket state before sending data. Without this check, the application could throw exceptions if the client disconnects unexpectedly, potentially causing middleware crashes.

    Medium
    Avoid potential deadlocks

    The code is using .Result to synchronously wait for an asynchronous operation,
    which can lead to deadlocks in ASP.NET applications. This is a common
    anti-pattern that should be avoided.

    src/Infrastructure/BotSharp.Core/Routing/RoutingContext.cs [85-93]

     // Convert id to name
     if (!Guid.TryParse(agentId, out _))
     {
         var agentService = _services.GetRequiredService<IAgentService>();
    -    var agents = agentService.GetAgentOptions([agentId]).Result;
    +    var agents = agentService.GetAgentOptions([agentId]).GetAwaiter().GetResult();
     
         if (agents.Count > 0)
         {
             agentId = agents.First().Id;

    [Suggestion has been applied]

    Suggestion importance[1-10]: 5

    __

    Why: The suggestion identifies a potential issue with using .Result, but the proposed solution of using GetAwaiter().GetResult() is only marginally better. While it avoids some deadlock scenarios, the method should ideally be refactored to be fully async with await.

    Low
    • Update

    @iceljc iceljc requested a review from Oceania2018 April 25, 2025 19:02
    {
    await hub.ConnectToModel(async data =>
    {
    await SendEventToUser(webSocket, data);

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Suggestion: Check WebSocket state

    Suggested change
    await SendEventToUser(webSocket, data);
    if (webSocket.State == WebSocketState.Open)
    {
    await SendEventToUser(webSocket, data);
    }

    convService.SetConversationId(conversationId, []);
    await convService.GetConversationRecordOrCreateNew(agentId);

    var buffer = new byte[1024 * 32];
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    image

    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.

    2 participants