Improved error handling for attempting to send too large message payl…#168
Improved error handling for attempting to send too large message payl…#168mattieruth merged 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves error handling for oversized message payloads and enhances error logging when no onError callback is registered. It introduces a new MessageTooLargeError exception class and validates message sizes before sending them through the transport layer.
Changes:
- Added MessageTooLargeError class for handling oversized messages
- Introduced message size validation with a 64 KB default limit
- Wrapped transport sendMessage calls with error handling and size validation
- Improved error logging to distinguish between missing callbacks and actual emit errors
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| client-js/rtvi/errors.ts | Added MessageTooLargeError class for handling oversized message payloads |
| client-js/client/utils.ts | Added messageSizeWithinLimit utility function to validate message sizes |
| client-js/client/transport.ts | Added _maxMessageSize field (64 KB) and maxMessageSize getter |
| client-js/client/client.ts | Introduced _sendMessage wrapper with validation, updated all sendMessage calls, improved error logging |
Comments suppressed due to low confidence (2)
client-js/client/client.ts:681
- The appendToContext method is declared as async and awaits the _sendMessage call, but _sendMessage is a synchronous void method. This creates an inconsistency where the await has no effect. Either _sendMessage should return a Promise, or this method should not be async and should not await the call.
await this._sendMessage(
new RTVIMessage(RTVIMessageType.APPEND_TO_CONTEXT, {
role: context.role,
content: context.content,
run_immediately: context.run_immediately,
} as LLMContextMessage)
);
client-js/client/client.ts:692
- The sendText method is declared as async and awaits the _sendMessage call, but _sendMessage is a synchronous void method. This creates an inconsistency where the await has no effect. Either _sendMessage should return a Promise, or this method should not be async and should not await the call.
await this._sendMessage(
new RTVIMessage(RTVIMessageType.SEND_TEXT, {
content,
options,
})
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private _sendMessage(message: RTVIMessage): void { | ||
| if (!messageSizeWithinLimit(message, this._transport.maxMessageSize)) { | ||
| const msg = | ||
| "Message data too large. Max size is " + this._transport.maxMessageSize; | ||
| this._options.callbacks?.onError?.(RTVIMessage.error(msg, false)); | ||
| throw new RTVIErrors.MessageTooLargeError(msg); | ||
| } | ||
|
|
||
| try { | ||
| this._transport.sendMessage(message); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| this._options.callbacks?.onError?.( | ||
| RTVIMessage.error(error.message, false) | ||
| ); | ||
| } else { | ||
| this._options.callbacks?.onError?.( | ||
| RTVIMessage.error("Unknown error sending message", false) | ||
| ); | ||
| } | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new messageSizeWithinLimit function and the _sendMessage wrapper that validates message size lack test coverage. Since the repository has existing tests for client functionality, tests should be added to cover message size validation, including cases for messages that exceed the limit and messages within the limit.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| export function messageSizeWithinLimit( | ||
| message: unknown, | ||
| maxSize: number | ||
| ): boolean { | ||
| const getSizeInBytes = (obj: unknown) => { | ||
| const jsonString = JSON.stringify(obj); | ||
| const encoder = new TextEncoder(); | ||
| const bytes = encoder.encode(jsonString); | ||
| return bytes.length; | ||
| }; | ||
| const size = getSizeInBytes(message); | ||
| return size <= maxSize; | ||
| } |
There was a problem hiding this comment.
The messageSizeWithinLimit utility function lacks test coverage. Since the repository has existing tests, tests should be added to verify that the size calculation is accurate for various message types and edge cases, such as messages with Unicode characters or nested objects.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
…oads Also improved error logging when onError handler is not registered.
82f125e to
af172dc
Compare
|
@mattieruth I've opened a new pull request, #169, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@mattieruth I've opened a new pull request, #170, to work on those changes. Once the pull request is ready, I'll request review from you. |
codepilot improved comments Co-authored-by: Copilot <[email protected]>
Co-authored-by: mattieruth <[email protected]>
b3a74cf to
70bd9a5
Compare
Co-authored-by: mattieruth <[email protected]>
Add test coverage for messageSizeWithinLimit utility function
| * typical WebSocket, proxy, and intermediary limits and to discourage | ||
| * transports from sending very large payloads in a single message. | ||
| * | ||
| * Transport implementations may override this value in subclasses or |
There was a problem hiding this comment.
Makes sense.
Are we going to override the DailyTransport to allow sending 10 MB ? Or do you think about keeping this limitation for all the transports for now.
There was a problem hiding this comment.
Because I guess if we introduce this without keeping the previous limits, it could cause issues for people who send large messages using Daily.
There was a problem hiding this comment.
filipi87
left a comment
There was a problem hiding this comment.
LGTM. 🚀
I think we just need to align the releases and also release the Daily transport with an increased message limit, so we don’t risk causing issues for users who send large messages using Daily.
…oads
Also improved error logging when onError handler is not registered.
This PR goes along with this transport PR