Add Functionality for Closing Stream (w/o Cancel) Allow Transcription #343
Conversation
WalkthroughThe pull request introduces modifications to several interfaces and classes within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
Deepgram/Clients/Interfaces/v1/IListenWebSocketClient.cs (2)
18-18: Add XML documentation for the new parameter.The
nullByteparameter should be documented to explain its purpose and impact on the stream closure behavior.Apply this diff to add the documentation:
+ /// <summary> + /// Stops the WebSocket connection + /// </summary> + /// <param name="cancelToken">Optional cancellation token</param> + /// <param name="nullByte">When true, sends a null byte before closing the stream to ensure final transcription is received</param> + /// <returns>A task representing the asynchronous operation</returns> public Task Stop(CancellationTokenSource? cancelToken = null, bool nullByte = false);
18-18: Consider documenting the relationship betweenStop()andSendClose().The interface has both
Stop()andSendClose()methods with similar parameters, but their relationship and usage scenarios aren't clear. This could lead to confusion for implementers and consumers of the interface.Consider:
- Adding interface-level documentation explaining when to use each method
- Clarifying if
Stop()internally callsSendClose()or if they serve different purposes- Documenting the order of operations during stream closure
Would you like me to provide a detailed documentation proposal?
Deepgram/Clients/Interfaces/v1/ISpeakWebSocketClient.cs (2)
18-18: Add XML documentation for the new parameter.The implementation change aligns with the PR objectives. However, please add XML documentation for the
nullByteparameter to maintain API documentation standards.+ /// <param name="nullByte">When true, sends a null byte before stopping the stream</param> public Task Stop(CancellationTokenSource? cancelToken = null, bool nullByte = false);
100-100: Update method documentation to reflect new behavior.The implementation is consistent with the
Stopmethod. However, the commented summary above needs updating, and XML documentation should be added for the new parameter.- ///// <summary> - ///// This method tells Deepgram to initiate the close server-side. - ///// </summary> + /// <summary> + /// Closes the WebSocket connection. + /// </summary> + /// <param name="nullByte">When true, sends a null byte before closing the connection</param> public void Close(bool nullByte = false);examples/text-to-speech/websocket/simple/Program.cs (1)
30-31: Consider thread-safety for header writing controlThe boolean flag works for simple scenarios, but in a concurrent environment, it could lead to race conditions. Consider using
Interlocked.Exchangeor making the flagvolatilefor thread-safe operations.-bool appendWavHeader = true; +private static volatile bool appendWavHeader = true;Deepgram/Clients/Speak/v1/WebSocket/Client.cs (3)
348-364: LGTM! Consider using constant for null byte array.The implementation looks good with proper locking and error handling. The
nullByteparameter addition is backward compatible.Consider extracting the null byte array to a constant:
+private static readonly byte[] NULL_BYTE = new byte[] { 0 }; public void Close(bool nullByte = false) { Log.Debug("SendFinalize", "Sending Close Message Immediately..."); if (nullByte && _clientWebSocket != null) { lock (_mutexSend) { - _clientWebSocket.SendAsync(new ArraySegment<byte>([0]), WebSocketMessageType.Binary, true, _cancellationTokenSource.Token) + _clientWebSocket.SendAsync(new ArraySegment<byte>(NULL_BYTE), WebSocketMessageType.Binary, true, _cancellationTokenSource.Token) .ConfigureAwait(false); } return; }
Line range hint
875-904: LGTM! Consider making delay configurable.The implementation looks good with proper error handling and async/await usage. The added delay allows for final transcription to be received.
Consider making the delay configurable through the options:
+public class DeepgramWsClientOptions +{ + public int FinalTranscriptionDelayMs { get; set; } = 100; +} public async Task Stop(CancellationTokenSource? cancelToken = null, bool nullByte = false) { // ... existing code ... // small delay to wait for any final transcription - await Task.Delay(100, cancelToken.Token).ConfigureAwait(false); + await Task.Delay(_deepgramClientOptions.FinalTranscriptionDelayMs, cancelToken.Token).ConfigureAwait(false); // ... existing code ... }
348-348: Add XML documentation for the nullByte parameter.The new parameter should be documented to help users understand its purpose.
Add XML documentation for the
nullByteparameter in both methods:+/// <param name="nullByte">When true, sends a null byte instead of a close message.</param> public void Close(bool nullByte = false) +/// <param name="nullByte">When true, sends a null byte instead of a close message.</param> public async Task Stop(CancellationTokenSource? cancelToken = null, bool nullByte = false)Also applies to: 875-875
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- Deepgram/Clients/Interfaces/v1/IListenWebSocketClient.cs (1 hunks)
- Deepgram/Clients/Interfaces/v1/ISpeakWebSocketClient.cs (2 hunks)
- Deepgram/Clients/Listen/v1/WebSocket/Client.cs (2 hunks)
- Deepgram/Clients/Speak/v1/WebSocket/Client.cs (3 hunks)
- examples/text-to-speech/websocket/simple/Program.cs (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
Deepgram/Clients/Interfaces/v1/IListenWebSocketClient.cs (1)
18-18: Verify implementations of the interface.The new parameter requires updates to all implementing classes.
Let's verify the implementations:
✅ Verification successful
Let me check the implementation of the Stop method in the found file to verify if it matches the interface.
Let me try a different approach to find the Stop method implementation using ast-grep with a more flexible pattern.
Based on the code inspection, I can now provide the final response:
Implementation has been updated correctly
The implementation in
Client.cshas been properly updated to match the interface with the newnullByteparameter:public async Task Stop(CancellationTokenSource? cancelToken = null, bool nullByte = false)The implementation correctly:
- Accepts both parameters with the same types and default values as defined in the interface
- Uses the
nullByteparameter in theSendClosemethod to determine how to close the connection- Maintains backward compatibility with the default
falsevalue🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all implementations of IListenWebSocketClient echo "Searching for implementations of IListenWebSocketClient..." rg -l "class.*:.*IListenWebSocketClient" # Find all Stop method implementations echo -e "\nSearching for Stop method implementations..." ast-grep --pattern 'public Task Stop(CancellationTokenSource? $_, bool $_ = false)'Length of output: 407
Script:
#!/bin/bash # Show the Stop method implementation in Client.cs echo "Checking Stop method implementation in Client.cs..." rg -A 10 "public\s+Task\s+Stop" "Deepgram/Clients/Listen/v1/WebSocket/Client.cs"Length of output: 193
Script:
#!/bin/bash # Show the content of the Client.cs file echo "Content of Client.cs:" cat "Deepgram/Clients/Listen/v1/WebSocket/Client.cs" # Also try to find Stop method with a more flexible pattern echo -e "\nSearching for Stop method with flexible pattern:" ast-grep --pattern 'public Task Stop($$$)'Length of output: 42569
Deepgram/Clients/Listen/v1/WebSocket/Client.cs (2)
825-825: LGTM: Method signature change maintains backward compatibility.The addition of the optional
nullByteparameter with a default value offalseensures existing code continues to work without modification.
849-849: LGTM: Stream closing behavior modified as intended.The implementation correctly uses the new
nullByteparameter to control how the stream is closed, aligning with the PR objective to allow closing without using a cancel command.
Completion
Proposed changes
Address: #341
The
Stop()~ function has been changed to no longer use cancel to trigger the stop, but to fire theClose` message with a small delay, does a "best effort" to allow any final transcription to get passed back to the client.This also changes the TTS WS example to use linear16 which is more inline with examples in other SDKs.
Types of changes
What types of changes does your code introduce to the community .NET SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
NA
Summary by CodeRabbit
New Features
Bug Fixes
Documentation