-
Notifications
You must be signed in to change notification settings - Fork 167
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
Add payload size limit #3017
base: master
Are you sure you want to change the base?
Add payload size limit #3017
Conversation
Corresponding BE PR: source-academy/backend#1151 |
Pull Request Test Coverage Report for Build 11011737335Details
💛 - Coveralls |
if (userInput.length > maxContentSize) { | ||
setMessages(prev => [...prev, MESSAGE_TOO_LONG_MESSAGE]); | ||
return; | ||
} |
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.
I don't think we should error when the user submits. Instead, the input field should not allow more than maxContentSize
characters.
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.
This can be a simple indicator of maxContentSize - userInput.length
and set the onChange
to ignore extra characters beyond maxContentSize
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.
Thanks for the comment. Feature added.
const MESSAGE_TOO_LONG_MESSAGE: Readonly<ChatMessage> = { | ||
content: 'Your message is too long. Please try again with a shorter message.', | ||
role: 'assistant' | ||
}; |
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.
That way, this will never be necessary. From UX perspective, it's easier for the user to edit their message before sending in order to fit the character limit, as opposed to having to copy and paste their previous message and send it again. Not to mention, if you do it like this, there will be a mismatch between the conversation history stored in the FE state with the one stored in the DB
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.
You are right;
it is more intuitive to do let the user trigger this in the first place;
this also pollute the chat history, making it less readable.
Description
Type of change
How to test
Checklist