fix: Resolve WebSocket issues and add real-time call example #160
Conversation
WalkthroughThis pull request introduces several modifications across multiple files in the Cozepy library, focusing on WebSocket event handling, chat functionality, and audio processing. The changes include updating event ID retrieval methods, adding a new chat update event type, modifying the Changes
Sequence DiagramsequenceDiagram
participant User
participant GUI
participant WebSocketClient
participant AudioStream
User->>GUI: Start Chat
GUI->>WebSocketClient: Initialize Connection
GUI->>AudioStream: Start Recording
AudioStream-->>WebSocketClient: Send Audio Data
WebSocketClient-->>GUI: Receive Chat Events
GUI->>AudioStream: Play Received Audio
User->>GUI: End Chat
GUI->>WebSocketClient: Close Connection
GUI->>AudioStream: Stop Recording
Possibly related PRs
Suggested Labels
Poem
Finishing Touches
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: 5
🧹 Nitpick comments (1)
examples/websockets_chat_realtime_gui.py (1)
28-32: Consider making audio parameters configurable.The hardcoded audio parameters could limit flexibility. Consider moving these to a configuration file or environment variables for easier adjustment.
-# 音频参数设置 -CHUNK = 1024 -FORMAT = pyaudio.paInt16 -CHANNELS = 1 -RATE = 24000 +# Audio parameters +CHUNK = int(os.getenv("AUDIO_CHUNK_SIZE", "1024")) +FORMAT = getattr(pyaudio, os.getenv("AUDIO_FORMAT", "paInt16")) +CHANNELS = int(os.getenv("AUDIO_CHANNELS", "1")) +RATE = int(os.getenv("AUDIO_RATE", "24000"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cozepy/chat/__init__.py(1 hunks)cozepy/websockets/audio/speech/__init__.py(2 hunks)cozepy/websockets/audio/transcriptions/__init__.py(3 hunks)cozepy/websockets/chat/__init__.py(9 hunks)cozepy/websockets/ws.py(6 hunks)examples/websockets_chat_realtime_gui.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
cozepy/websockets/ws.py
[error] 562-562: "WebsocketsEvent" has no attribute "_dump_without_delta"
🔇 Additional comments (9)
cozepy/websockets/audio/speech/__init__.py (1)
124-124: LGTM! Event ID retrieval standardization.The change from
message.get("event_id")tomessage.get("id")aligns with the standardization of event ID retrieval across the codebase.Also applies to: 238-238
cozepy/websockets/audio/transcriptions/__init__.py (1)
33-41: LGTM! Efficient event logging implementation.The
_dump_without_deltamethod provides an efficient way to log events by including the length of audio data instead of the data itself.cozepy/websockets/chat/__init__.py (3)
61-65: LGTM! Well-structured event class implementation.The new
ChatUpdatedEventclass follows the established pattern and maintains consistency with other event classes.
116-117: LGTM! Consistent event handler implementation.The
on_chat_updatedmethod is properly implemented in both sync and async event handlers.Also applies to: 320-321
212-219: LGTM! Proper event loading implementation.The
_load_eventmethod correctly handles the newCHAT_UPDATEDevent type in both sync and async clients.Also applies to: 428-435
cozepy/websockets/ws.py (3)
96-96: LGTM: New event type added correctlyThe new
CHAT_UPDATEDevent type is properly added to the WebSocket events enum, following the established naming convention.
113-113: LGTM: Field renamed for API consistencyThe field rename from
event_idtoidmaintains type safety while improving API consistency.
122-122: LGTM: Simplified model inheritanceChanged to inherit directly from pydantic's BaseModel, which is appropriate as this class doesn't need the additional functionality from CozeModel.
cozepy/chat/__init__.py (1)
194-194: LGTM: Improved return type consistencyChanged to return empty bytes (
b"") instead ofNone, which is a better practice for methods handling binary data. This ensures type consistency and simplifies error handling in calling code.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #160 +/- ##
==========================================
- Coverage 90.26% 90.13% -0.13%
==========================================
Files 63 63
Lines 5732 5748 +16
==========================================
+ Hits 5174 5181 +7
- Misses 558 567 +9
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cozepy/websockets/ws.py (1)
557-565:⚠️ Potential issueCritical: Fix undefined method
_dump_without_delta.The code attempts to call
_dump_without_delta()on the event object, but this method is not defined in theWebsocketsEventclass.Apply this diff to fix the issue:
- json.dumps(event._dump_without_delta()), # type: ignore + event.model_dump_json(),🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 557-558: cozepy/websockets/ws.py#L557-L558
Added lines #L557 - L558 were not covered by tests
[warning] 565-565: cozepy/websockets/ws.py#L565
Added line #L565 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)cozepy/chat/__init__.py(1 hunks)cozepy/websockets/audio/speech/__init__.py(2 hunks)cozepy/websockets/audio/transcriptions/__init__.py(3 hunks)cozepy/websockets/chat/__init__.py(9 hunks)cozepy/websockets/ws.py(6 hunks)examples/websockets_chat_realtime_gui.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/websockets_chat_realtime_gui.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cozepy/websockets/audio/transcriptions/__init__.py
[warning] 34-34: cozepy/websockets/audio/transcriptions/init.py#L34
Added line #L34 was not covered by tests
[warning] 140-140: cozepy/websockets/audio/transcriptions/init.py#L140
Added line #L140 was not covered by tests
[warning] 263-263: cozepy/websockets/audio/transcriptions/init.py#L263
Added line #L263 was not covered by tests
cozepy/websockets/chat/__init__.py
[warning] 117-117: cozepy/websockets/chat/init.py#L117
Added line #L117 was not covered by tests
[warning] 201-201: cozepy/websockets/chat/init.py#L201
Added line #L201 was not covered by tests
[warning] 212-213: cozepy/websockets/chat/init.py#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 321-321: cozepy/websockets/chat/init.py#L321
Added line #L321 was not covered by tests
[warning] 417-417: cozepy/websockets/chat/init.py#L417
Added line #L417 was not covered by tests
[warning] 428-429: cozepy/websockets/chat/init.py#L428-L429
Added lines #L428 - L429 were not covered by tests
cozepy/chat/__init__.py
[warning] 194-194: cozepy/chat/init.py#L194
Added line #L194 was not covered by tests
cozepy/websockets/audio/speech/__init__.py
[warning] 124-124: cozepy/websockets/audio/speech/init.py#L124
Added line #L124 was not covered by tests
[warning] 238-238: cozepy/websockets/audio/speech/init.py#L238
Added line #L238 was not covered by tests
cozepy/websockets/ws.py
[warning] 270-270: cozepy/websockets/ws.py#L270
Added line #L270 was not covered by tests
[warning] 470-470: cozepy/websockets/ws.py#L470
Added line #L470 was not covered by tests
[warning] 557-558: cozepy/websockets/ws.py#L557-L558
Added lines #L557 - L558 were not covered by tests
[warning] 565-565: cozepy/websockets/ws.py#L565
Added line #L565 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (Python 3.8 on macOS)
- GitHub Check: test (Python 3.8 on Windows)
🔇 Additional comments (17)
cozepy/websockets/chat/__init__.py (7)
163-163: RegisterCHAT_UPDATEDEvent inon_eventMappingThe
CHAT_UPDATEDevent is correctly registered in theon_eventmapping ofWebsocketsChatClient. This ensures that the event handler will processChatUpdatedEventappropriately.
379-379: RegisterCHAT_UPDATEDEvent in Asyncon_eventMappingThe
CHAT_UPDATEDevent is correctly registered in theon_eventmapping ofAsyncWebsocketsChatClient. This ensures that asynchronous event handling forChatUpdatedEventis properly set up.
61-64: 🛠️ Refactor suggestionAdd Unit Tests for
ChatUpdatedEventClassThe newly added
ChatUpdatedEventclass lacks test coverage, as indicated by static analysis tools. To ensure reliability and prevent regressions, please add unit tests for this class.Would you like me to help generate unit tests for this class or open a GitHub issue to track this task?
116-118: 🛠️ Refactor suggestionAdd Unit Tests for
on_chat_updatedMethodThe
on_chat_updatedmethod inWebsocketsChatEventHandleris not covered by tests. Adding unit tests will help verify that it handlesChatUpdatedEventcorrectly.Would you like assistance in creating unit tests for this method or opening a GitHub issue to track this?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 117-117: cozepy/websockets/chat/init.py#L117
Added line #L117 was not covered by tests
201-201: 🛠️ Refactor suggestionUpdate Event ID Retrieval and Handle
CHAT_UPDATEDEventUpdating
event_idretrieval frommessage.get("event_id")tomessage.get("id")aligns with the new message format. The addition of handling for theCHAT_UPDATEDevent in the_load_eventmethod is correctly implemented.However, static analysis indicates that these changes are not covered by tests. Please ensure that unit tests are added to cover these modifications to maintain code quality.
Do you need help in writing unit tests for these changes or creating a GitHub issue to address this?
Also applies to: 212-219
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 201-201: cozepy/websockets/chat/init.py#L201
Added line #L201 was not covered by tests
320-322: 🛠️ Refactor suggestionAdd Unit Tests for Async
on_chat_updatedMethodThe asynchronous
on_chat_updatedmethod inAsyncWebsocketsChatEventHandlerlacks test coverage. Adding unit tests will help ensure it functions as intended.Would you like assistance in creating unit tests for this async method or opening a GitHub issue to track this?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 321-321: cozepy/websockets/chat/init.py#L321
Added line #L321 was not covered by tests
417-417: 🛠️ Refactor suggestionUpdate Event ID Retrieval and Handle
CHAT_UPDATEDEvent in Async ClientConsistent with the synchronous client, the event ID retrieval is updated, and handling for the
CHAT_UPDATEDevent is added in the asynchronous client's_load_eventmethod.Static analysis tools indicate that these changes are not covered by tests. Please add unit tests to verify the correct behavior of these modifications.
Do you need help in writing unit tests for these asynchronous changes or creating a GitHub issue to address this?
Also applies to: 428-435
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 417-417: cozepy/websockets/chat/init.py#L417
Added line #L417 was not covered by testscozepy/websockets/audio/speech/__init__.py (2)
238-238: Update Event ID Retrieval in Async ClientThe asynchronous client now retrieves
event_idusingmessage.get("id"), matching the synchronous client and the updated message format.Please run the same verification script to ensure consistency across the codebase.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 238-238: cozepy/websockets/audio/speech/init.py#L238
Added line #L238 was not covered by tests
124-124: Update Event ID Retrieval tomessage.get("id")Changing
event_idretrieval frommessage.get("event_id")tomessage.get("id")aligns with the updated message format. This ensures consistency in event handling.Run the following script to verify that all occurrences of
message.get("event_id")have been updated:✅ Verification successful
Event ID Retrieval Pattern is Consistent
The change to
message.get("id")is verified to be consistent with all other websocket handlers in the codebase. No instances of the oldmessage.get("event_id")pattern remain.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any remaining instances of 'message.get("event_id")' in the codebase. rg 'message\.get\("event_id"\)'Length of output: 33
Script:
#!/bin/bash # Search with broader patterns and in specific locations # Look for any occurrence of "event_id" in the codebase echo "=== Searching for 'event_id' ===" rg -i "event_id" # Look for current pattern message.get("id") echo -e "\n=== Searching for message.get(\"id\") ===" rg 'message\.get\("id"\)' # Focus search in the websockets/audio/speech directory echo -e "\n=== Searching in websockets/audio/speech directory ===" fd . "websockets/audio/speech" --type f --exec rg -i "event.?id|message.*id"Length of output: 6060
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 124-124: cozepy/websockets/audio/speech/init.py#L124
Added line #L124 was not covered by testscozepy/websockets/audio/transcriptions/__init__.py (3)
33-42: Introduce_dump_without_deltaMethod for Efficient LoggingThe
_dump_without_deltamethod inInputAudioBufferAppendEventprovides a way to serialize the event without the potentially largedeltadata. This is useful for logging or debugging purposes without incurring performance overhead.Ensure that this method is appropriately utilized where necessary. Consider adding unit tests to cover this method and verify its functionality.
Would you like me to help write unit tests for this method or open a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: cozepy/websockets/audio/transcriptions/init.py#L34
Added line #L34 was not covered by tests
263-263: Update Event ID Retrieval in Async ClientThe asynchronous client now retrieves the event ID using
message.get("id"), ensuring consistency with the synchronous client and updated event message format.Ensure that this change is covered by unit tests to prevent potential issues.
Would you like assistance in adding unit tests for this change or creating a GitHub issue to track it?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 263-263: cozepy/websockets/audio/transcriptions/init.py#L263
Added line #L263 was not covered by tests
140-140: Update Event ID Retrieval tomessage.get("id")Updating the event ID retrieval to
message.get("id")in the_load_eventmethod aligns with changes in the event message format. This ensures that the correct event ID is used during event processing.Please verify that all instances of
message.get("event_id")have been updated throughout the codebase.Run the following script to check for any remaining outdated instances:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 140-140: cozepy/websockets/audio/transcriptions/init.py#L140
Added line #L140 was not covered by testscozepy/websockets/ws.py (4)
96-96: LGTM! New event type follows naming convention.The addition of
CHAT_UPDATEDevent type aligns with the WebSocket enhancements.
122-122: Verify impact of inheritance change.The change from
CozeModeltoBaseModelforInputAudioclass could affect validation and serialization behavior.✅ Verification successful
Inheritance change from CozeModel to BaseModel is safe ✅
The change is safe because:
- InputAudio only uses basic Pydantic features (optional fields)
- No CozeModel-specific features are being used
- CozeModel itself inherits from BaseModel, so no essential functionality is lost
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of CozeModel-specific features in InputAudio class. # Test: Search for any CozeModel-specific method calls or features used with InputAudio. echo "Searching for InputAudio usage patterns..." rg -A 5 'InputAudio\(' --type python echo "Checking for CozeModel method calls on InputAudio instances..." ast-grep --pattern $'$VAR = InputAudio($$$) $VAR.$METHOD($$$)'Length of output: 363
Script:
#!/bin/bash # First, find CozeModel definition to understand what features might be lost echo "Searching for CozeModel definition..." rg -t py "class CozeModel" echo "Searching for InputAudio usage patterns..." rg -t py -A 5 "InputAudio\(" echo "Searching for any files containing both InputAudio and CozeModel..." rg -t py -l "InputAudio.*CozeModel|CozeModel.*InputAudio" # Look for the complete InputAudio class definition echo "Searching for InputAudio class definition..." ast-grep --pattern 'class InputAudio($_) { $$$ }'Length of output: 952
113-113: Verify consistent usage of renamed field.The field rename from
event_idtoidinWebsocketsEventclass requires verification of consistent usage across the codebase.✅ Verification successful
Field rename verified successfully
The rename from
event_idtoidinWebsocketsEventclass is consistent across the codebase. The remainingevent_idoccurrence in workflow_stream.py is unrelated as it belongs to a different data structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of renamed field across the codebase. # Test: Search for any remaining occurrences of 'event_id' that might need updating. echo "Searching for remaining 'event_id' occurrences..." rg -A 3 'event_id' --type python echo "Verifying 'id' field usage in WebsocketsEvent..." ast-grep --pattern $'class $CLASS extends WebsocketsEvent { $$$ event_id: $TYPE $$$ }'Length of output: 385
Script:
#!/bin/bash # Search for both old and new field names in Python files echo "Searching for 'event_id' occurrences..." rg -l '\.event_id' --type-add 'py:*.py' -t py echo "Searching for WebsocketsEvent class usage..." rg -A 5 'class.*WebsocketsEvent' --type-add 'py:*.py' -t py echo "Searching for field access patterns..." rg '\.(event_id|id)\s*[=:]' --type-add 'py:*.py' -t pyLength of output: 13439
Script:
#!/bin/bash # Check the specific usage in workflow_stream.py echo "Examining event_id usage in workflow_stream.py..." rg -A 3 'event_id' examples/workflow_stream.pyLength of output: 354
270-270: Add test coverage for_load_all_eventmethod.The method is not covered by tests according to code coverage report.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 270-270: cozepy/websockets/ws.py#L270
Added line #L270 was not covered by testscozepy/chat/__init__.py (1)
194-194: Verify handling of empty audio data.The change from returning
Nonetob""ensures consistent return type but may affect code that checks forNone. Also, the method lacks test coverage.✅ Verification successful
The change from
Nonetob""is safe based on current usage patternsThe two files using
get_audio()handle bothNoneand empty bytes correctly:
examples/websockets_chat_realtime_gui.pychecks truthiness before using the resultexamples/websockets_chat.pyappends the result to a list, which works with both values🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of get_audio method and existing test coverage. # Test: Search for get_audio method calls and related test cases. echo "Searching for get_audio method calls..." rg -A 3 'get_audio\(\)' --type python echo "Searching for get_audio test cases..." rg -A 5 'test.*get_audio' --type pythonLength of output: 317
Script:
#!/bin/bash # Search for get_audio method calls with proper context echo "=== Searching for get_audio method calls ===" rg -A 3 '\.get_audio\(\)' --type py echo -e "\n=== Searching for get_audio test cases ===" rg -A 5 '(test.*get_audio|get_audio.*test)' --type py echo -e "\n=== Searching for get_audio method definition ===" rg -B 5 -A 10 'def get_audio' --type pyLength of output: 2886
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 194-194: cozepy/chat/init.py#L194
Added line #L194 was not covered by tests
| @@ -466,7 +467,7 @@ | |||
| await self._handle_error(e) | |||
|
|
|||
| def _load_all_event(self, message: Dict) -> Optional[WebsocketsEvent]: | |||
| event_id = message.get("event_id") or "" | |||
| event_id = message.get("id") or "" | |||
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for both sync and async _load_all_event methods
The websocket event parsing methods are not covered by tests. These methods handle critical functionality:
- Core message field parsing with fallbacks
- Event detail validation
- Error event handling
🔗 Analysis chain
Add test coverage for async _load_all_event method.
The async version of the method is not covered by tests according to code coverage report.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing test coverage for async _load_all_event method.
# Test: Search for test files and test cases related to async _load_all_event.
echo "Searching for async test files..."
fd -e py test_.*async.*\.py
echo "Searching for async _load_all_event test cases..."
rg -A 5 'async.*_load_all_event' --type python
Length of output: 296
Script:
#!/bin/bash
# Search for test files and _load_all_event references
echo "=== Test files ==="
fd -e py test_
echo -e "\n=== _load_all_event references in tests ==="
rg "_load_all_event" -t py tests/
echo -e "\n=== Method definition ==="
rg -B2 -A5 "def.*_load_all_event" -t py
Length of output: 2108
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 470-470: cozepy/websockets/ws.py#L470
Added line #L470 was not covered by tests
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
.gitignorefor specific audio response files.