feat(ai) add attachment validations #6211
Conversation
🦋 Changeset detectedLatest commit: d86499f The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // We save attachments as base64 strings into dynamodb for conversation history | ||
| // DynamoDB has a max size of 400kb for records | ||
| // This can be overridden so cutsomers could provide a lower number | ||
| // or a higher number if in the future we support larger sizes. | ||
| maxAttachmentSize = 400_000, | ||
| maxAttachments = 20, |
There was a problem hiding this comment.
Are these safe to override today?
There was a problem hiding this comment.
They are safe to override to be lower, but if they are higher the customer will probably get an error further down in the stack (in lambda)
There was a problem hiding this comment.
Got it. Lmk if I missed it, but are we validating that the provided values are not larger than the known thresholds?
There was a problem hiding this comment.
We aren't validating if the provided values are larger than the known thresholds. I did this because the thresholds will go up at a later point, the 400kb size limit per file is a limitation of our backend implementation and not Bedrock or the AI model's, and we do plan to fix this long term. I can change it so we enforce the limits though if we feel strongly about it.
packages/react-ai/src/components/AIConversation/context/ControlsContext.tsx
Outdated
Show resolved
Hide resolved
packages/react-ai/src/components/AIConversation/views/default/Form.tsx
Outdated
Show resolved
Hide resolved
| for (const file of files) { | ||
| const arrayBuffer = await file.arrayBuffer(); | ||
| const base64 = arrayBufferToBase64(arrayBuffer); |
There was a problem hiding this comment.
Nit: Could this be done as Promise.all?
| maxAttachmentSize, | ||
| }); | ||
|
|
||
| if (hasMaxError ?? hasMaxSizeError) { |
There was a problem hiding this comment.
Should this be ||? If hasMaxError ever becomes false (maybe due to changes to code in the future) instead of being nullish, it could hide the hasMaxSizeError
| acceptedFiles: File[]; | ||
| rejectedFiles: File[]; | ||
| hasMaxSizeError?: boolean; | ||
| hasMaxError?: boolean; |
There was a problem hiding this comment.
Nit: Can we consider changing this to hasMaxAttachmentsError or something? It's not immediately clear what "max" is referring to
| error += displayText.getMaxAttachmentErrorText(maxAttachments); | ||
| } | ||
| if (hasMaxSizeError) { | ||
| error += displayText.getAttachmentSizeErrorText( |
There was a problem hiding this comment.
If both errors are present, is the string just joined together without a space in between? Would it be better to do something like
const errors = [];
errors.push(maxAttachmentError);
errors.push(sizeError);
setError(errors.join(' '));
Description of changes
Issue #, if available
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.