-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add sendUserMessage method to AnamClient #143
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
Conversation
Smidge
commented
Jul 29, 2025
- Add sendHumanMessage() method for sending human text messages
- Method checks streaming status and active session
- Formats message with timestamp, session_id, and message_type
- Add JSDoc documentation for talk() and sendDataMessage() methods
Improve compatibility for older browsers and react native
- Add sendHumanMessage() method for sending human text messages - Method checks streaming status and active session - Formats message with timestamp, session_id, and message_type - Add JSDoc documentation for talk() and sendDataMessage() methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new sendHumanMessage method to the AnamClient class for sending human text messages during active streaming sessions. The method validates streaming status and session state before formatting and sending messages through the WebRTC data channel.
- Add
sendHumanMessage()method with validation for streaming status and active session - Add JSDoc documentation for existing
talk()andsendDataMessage()methods - Reorganize imports for better structure
| throw new Error('Failed to send human message: no active session'); | ||
| } | ||
|
|
||
| const currentTimestamp = new Date().toISOString().replace('Z', ''); |
Copilot
AI
Jul 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timestamp format removes the 'Z' suffix but doesn't replace it with anything, resulting in an incomplete ISO timestamp. This could cause issues with timestamp parsing. Consider using a proper timezone offset or keeping the 'Z' for UTC.
| const currentTimestamp = new Date().toISOString().replace('Z', ''); | |
| const currentTimestamp = new Date().toISOString(); |
| console.warn( | ||
| 'AnamClient: Not currently streaming. Human message will not be sent.', | ||
| ); |
Copilot
AI
Jul 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using console.warn followed by throwing an error creates redundant messaging. Consider either logging a warning and returning early, or throwing the error without the console warning to avoid duplicate error reporting.
| console.warn( | |
| 'AnamClient: Not currently streaming. Human message will not be sent.', | |
| ); |
| content, | ||
| timestamp: currentTimestamp, | ||
| session_id: sessionId, | ||
| message_type: 'speech', |
Copilot
AI
Jul 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded 'speech' message type as a magic string should be extracted to a constant or enum to improve maintainability and prevent typos.
| message_type: 'speech', | |
| message_type: MESSAGE_TYPE_SPEECH, |
- Rename method from sendHumanMessage() to sendUserMessage() - Update JSDoc documentation to use 'user' instead of 'human' - Update error messages and console warnings accordingly