feat: support file upload in agent chat - #2429
Conversation
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
input.mp4 |
|
Tested locally with a PDF and gpt-4.1-mini: |
github-actions
Bot
added
enhancement
| } = {}, | ||
| ) => { | ||
| if (!userMessageText.trim() || !selectedAgentName || !selectedNamespace) { | ||
| const filesToSend = pendingFiles; |
There was a problem hiding this comment.
handleMcpAppSendMessage() also calls sendChatMessageText, so an MCP app's ui/message will send and clear any files the user has staged.
|
|
||
| if (options.clearInput ?? true) { | ||
| setCurrentInputMessage(""); | ||
| setPendingFiles([]); |
There was a problem hiding this comment.
If the cross-tab guard below returns "blocked", then the staged files will be cleared without sending. We should only clear after the cross-tab guard succeeds.
| Source: &types.ImageSourceMemberBytes{Value: part.InlineData.Data}, | ||
| }, | ||
| }) | ||
| } else if docFmt := bedrockDocumentFormat(mime, name); docFmt != "" { |
There was a problem hiding this comment.
Based on the Bedrock Converse documentation, if a message contains a document content block, there must also be a text content block:
If you include a ContentBlock with a document field in the array, you must also include a ContentBlock with a text field.
There was a problem hiding this comment.
I can think of two ways to enforce this, either we allow the user to only send a document (which is the behaviour right now) and check in adapter if there is no text part, we will add a default message like "review this file", or we will require the user to always send a text part alongside any documents (excluding images, since bedrock only requires this for documents).
I'll go with the first approach since A2A allows FilePart without a TextPart in messages
| } from "@/lib/chatSessionGuard"; | ||
|
|
||
| /** Soft client cap so base64 A2A FileParts stay reasonable. */ | ||
| const MAX_CHAT_FILE_BYTES = 10 * 1024 * 1024; |
There was a problem hiding this comment.
Bedrock has a max size of: 3.75 MB for images and 4.5 MB for documents with a max of 20 images or 5 documents. We should make sure the count/size conform to provider limits before allowing sending.
- You can include up to 20 images. Each image’s size, height, and width must be no more than 3.75 MB, 8000 px, and 8000 px, respectively.
- You can include up to five documents. Each document’s size must be no more than 4.5 MB.
There was a problem hiding this comment.
Agreed, I've added the checks and the above 2 UI review comments in 2e28e44
github-actions
Bot
added
enhancement
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
github-actions
Bot
added
enhancement
iplay88keys
left a comment
iplay88keys
left a comment
There was a problem hiding this comment.
Just some extra validation, but nothing blocking.
| if (file.size > maxBytes) { | ||
| const mb = (maxBytes / (1024 * 1024)).toFixed(2); | ||
| toast.error(`${file.name} exceeds the ${mb}MB ${isImage ? "image" : "file"} limit`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Bedrock supports images up to a max size of 8000 x 8000 px. Do we want to validate that here as well?
| func bedrockSafeDocName(name string) string { | ||
| if name == "" { | ||
| return "document" | ||
| } | ||
| var b strings.Builder | ||
| prevSpace := false | ||
| for _, r := range name { | ||
| switch { | ||
| case (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || | ||
| r == '-' || r == '(' || r == ')' || r == '[' || r == ']': | ||
| b.WriteRune(r) | ||
| prevSpace = false | ||
| case r == ' ' || r == '_' || r == '.': | ||
| if !prevSpace && b.Len() > 0 { | ||
| b.WriteByte(' ') | ||
| prevSpace = true | ||
| } | ||
| } | ||
| } | ||
| out := strings.TrimSpace(b.String()) | ||
| if out == "" { | ||
| return "document" | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
Bedrock document names also have a max of 200 characters. It's also worth considering a neutral name for them, though we'd have to distinctly name each of the files.
From the docs:
A name for the document. The name can only contain the following characters:
- Alphanumeric characters
- Whitespace characters (no more than one in a row)
- Hyphens
- Parentheses
- Square brackets
Note
This field is vulnerable to prompt injections, because the model might inadvertently interpret it as instructions. Therefore, we recommend that you specify a neutral name.Constraints:
- min: 1
- max: 200
| const handleAttachFiles = async (fileList: FileList | null) => { | ||
| if (!fileList || fileList.length === 0) return; | ||
| const next: PendingChatFile[] = []; | ||
| let slots = MAX_CHAT_FILES - pendingFiles.length; |
There was a problem hiding this comment.
Codex's review mentions:
pendingFiles.lengthis captured before the asynchronous file reads. Two overlapping picker/drop operations can both calculate the same available slots and ultimately append more thanMAX_CHAT_FILES. Please reserve slots while reading or enforce the cap atomically in the state update.
Current support matrix: