Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Jul 25, 2025

PR Type

Enhancement


Description

  • Add logging for Twilio stream connection lifecycle events

  • Implement error handling for JSON deserialization failures

  • Include conversation ID in log messages for better traceability


Diagram Walkthrough

flowchart LR
  A["WebSocket Connection"] --> B["MapEvents Method"]
  B --> C["JSON Deserialization"]
  C --> D["Error Handling & Logging"]
  D --> E["Connection Lifecycle Logging"]
  E --> F["User Connected/Disconnected Events"]
Loading

File Walkthrough

Relevant files
Enhancement
TwilioStreamMiddleware.cs
Enhanced logging and error handling for stream events       

src/Plugins/BotSharp.Plugin.Twilio/TwilioStreamMiddleware.cs

  • Add logging for user connection and disconnection events
  • Implement try-catch error handling for JSON deserialization
  • Include conversation ID parameter in MapEvents method
  • Add conditional logging levels for DEBUG vs production builds
+24/-3   

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

Null Reference

After catching JSON deserialization exception, the response object remains null but is still accessed on line 158 without null checking, which will cause a NullReferenceException.

conn.StreamId = response.StreamSid;
string eventType = response.Event;
Log Level Issue

Using LogCritical for normal connection events in DEBUG mode is inappropriate as it suggests system failure rather than normal operation flow.

                _logger.LogCritical($"Start twilio stream connection for conversation ({conversationId})");
#else

@iceljc iceljc merged commit 0317368 into SciSharp:master Jul 25, 2025
3 of 4 checks passed
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle deserialization failure properly

The code continues execution after deserialization failure, which will cause
null reference exceptions when accessing response.StreamSid and response.Event.
Add early return or proper null handling after catching the exception.

src/Plugins/BotSharp.Plugin.Twilio/TwilioStreamMiddleware.cs [147-158]

-StreamEventResponse? response = new();
+StreamEventResponse? response = null;
 
 try
 {
     response = JsonSerializer.Deserialize<StreamEventResponse>(receivedText);
 }
 catch (Exception ex)
 {
     _logger.LogError(ex, $"Error when deserializing twilio stream event response for conversation ({conversationId}) (response: {receivedText?.SubstringMax(30)})");
+    return ("error", string.Empty);
 }
 
 conn.StreamId = response.StreamSid;
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug where a JSON deserialization failure is caught but not handled, leading to incorrect program flow with default data.

High
General
Use appropriate debug logging level

Using LogCritical for normal connection events in DEBUG builds is misleading and
will pollute critical error monitoring. Use LogDebug or LogTrace instead for
debug-specific verbose logging.

src/Plugins/BotSharp.Plugin.Twilio/TwilioStreamMiddleware.cs [96-100]

 #if DEBUG
-                _logger.LogCritical($"Start twilio stream connection for conversation ({conversationId})");
+                _logger.LogDebug($"Start twilio stream connection for conversation ({conversationId})");
 #else
                 _logger.LogInformation($"Start twilio stream connection for conversation ({conversationId})");
 #endif
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that using LogCritical for a standard connection event is semantically incorrect and proposes using the more appropriate LogDebug level.

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