fix(site/src/pages/AgentsPage): inline text-file attachments as text#26556
fix(site/src/pages/AgentsPage): inline text-file attachments as text#26556johnstcn wants to merge 4 commits into
Conversation
Text-family uploads (JSON, CSV, Markdown, plain text) were sent as provider file parts and silently dropped by providers that only accept images and PDFs. Read their contents in the browser and send them as a text part so every provider sees the content.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.8.0 | Round 4 | Last posted: Round 4, 13 findings (2 P2, 6 P3, 4 Nit, 1 Note), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-4 (P3, messageParsing.ts:329) - Edit path joins inlined text parts unseparated with user message
Round logRound 1Netero-only. 1 P2. Reviewed against 4aa2482..1ba5ae8. Round 2Panel (14 reviewers). CRF-1 addressed. 1 P2, 5 P3, 3 Nit, 1 Note new. 2 dropped. Reviewed against 4aa2482..0e92730. Round 3Panel (6 reviewers). 9 addressed, CRF-4 contested and closed (3/5). 1 P3, 1 Nit new. Reviewed against 4aa2482..003289e. Round 4Netero-only. CRF-14, CRF-15 addressed. No new findings. All 15 findings resolved. APPROVE. Reviewed against 4aa2482..bf9da5f. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
First-pass review (Netero). 1 P2 finding.
The change solves a real problem cleanly: text-family uploads that providers silently drop are inlined as text parts so every provider sees the content. The helper extraction into chatAttachments.ts is well-factored, the set is derived from the generated allowlist, and the test coverage is solid (33 tests, extension fallback included).
One error-handling gap in the send path needs attention before the full review panel runs.
This is a first-pass review only. These are mechanical findings; the full review panel has not yet reviewed this PR. The panel will review after this finding is addressed.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (Round 2). CRF-1 (unguarded await) verified fixed. 1 P2, 5 P3, 3 Nit, 1 Note.
The approach is sound: text-family uploads that providers silently drop are inlined as text parts so every provider sees the content. The helper extraction is clean, the set derivation from the generated allowlist keeps it in sync automatically, and the test coverage is thorough (33 tests). The CRF-1 fix is well-structured with correct fallback semantics.
The main gap is that the fix was applied to the chat page's send handler but not the create form's send handler, which constructs file parts the same way and hits the same silent drop.
"Both paths are in the same directory, use the same hook, and render the same AgentChatInput component. A grep for type: file, file_id would have surfaced this." (Mafu-san)
site/src/pages/AgentsPage/components/AgentCreateForm.tsx:391
P2 [CRF-2] Create form send path not updated; text-family attachments are silently dropped on new chat creation.
handleSendWithAttachments (line 391) collects only state.fileId strings and passes them as fileIDs to handleSend. In AgentCreatePage.tsx:89-92, each ID becomes { type: "file", file_id: fileID }. No check for isInlinableTextAttachment, no reading of textContents (destructured at line 380 but unused).
The hook change in this PR broadened text reading to the full inlinable text family for both consumers. The chat page's send handler was updated to use that text content. This send handler ignores it. A user creating a new chat with a .json attachment from the create form still hits the silent provider drop.
(Mafu-san)
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (Round 3). All R2 findings addressed or closed. 1 P3, 1 Nit new.
The R3 fixup is a well-executed structural response to the R2 findings. Rather than patching each finding individually, the author extracted resolvePendingAttachments as shared logic, eliminating the create-form gap (CRF-2), the duplicate reader (CRF-3), and the silent catch (CRF-5) in one consolidation. Both send paths now call the same function. 38 tests pass across both test files.
CRF-4 (edit path joins text parts unseparated) was contested and closed by the panel (3/5 accept). The defense holds: .join("") mirrors the server's string_agg, the schema has no field to distinguish user-typed text from inlined content, and the behavior is lossless. The edit-path UX degradation (no file chip, attached content mixed into the editor) is a known trade-off. If this matters, it needs a human decision: file a ticket for a content-format marker or accept the gap.
"The resolvePendingAttachments extraction is a clean refactor that unifies both submit paths. Boring and correct." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All findings resolved. Approving.
Across 4 rounds this PR addressed 2 P2s (unguarded await in send handler, create-form send path gap), 5 P3s, 3 Nits, and 1 Note. One contested finding (edit-path text joining) was closed by the panel after the author demonstrated it mirrors the server-side aggregation contract.
The final shape is clean: resolvePendingAttachments unifies both send paths, text-family uploads are inlined as text parts so every provider sees the content, error fallback logs a console.warn and degrades to a file part, and 39 tests cover classification, formatting, on-demand read, read failure, restored zero-byte files, and upload-error skipping. Test density is 49%.
CRF-4 (edit-path UX degradation for inlined text attachments) is a known trade-off documented in the PR's Known Limitations section. A human decision is needed on whether to file a ticket for a content-format marker or accept the gap.
🤖 This review was automatically generated with Coder Agents.
Problem
Attaching a non-image/non-PDF file (for example a
.jsonfile) to a Coder Agents chat uploads and stores the file correctly, but the attachment is silently dropped before the model ever sees it.Root cause: the message is sent as a
filepart referencing the uploaded file. Each provider's conversion layer in thefantasySDK only accepts a small set of file media types (OpenAI Responses:image/*,application/pdf; Anthropic:image/*,application/pdf,text/*). Anything else, includingapplication/json, is discarded with aCallWarningthat is recorded in the run'snormalized_response.warningsbut never surfaced to the user. Switching models does not help.Fix
Read text-family uploads (
text/plain,text/markdown,text/csv,application/json) in the browser and send their contents to the model as an inline text part instead of a droppable providerfilepart. Text parts are accepted by every provider, so the content always reaches the model. Images and PDFs are unchanged and still sent as file parts.This is intentionally frontend-only. The server remains authoritative on stored media types (it byte-sniffs and enforces an allowlist), so no backend behavior changes.
Changes (all under
site/src/pages/AgentsPage/)utils/chatAttachments.ts: new pure helpersisInlinableTextAttachment(text family derived from the generatedChatAttachmentMediaTypes, with a filename-extension fallback that reuseschatAttachmentExtraExtensions),formatInlinedAttachmentText, andattachmentToContentPart; thePendingAttachmenttype now lives here.utils/resolvePendingAttachments.ts: new shared resolver used by both send paths. It turns uploaded attachments into the send payload, inlining text-family files as text parts, reading on demand when needed, and falling back to a file part (with aconsole.warn) if the read fails.hooks/useFileAttachments.ts: broadened the existing content read fromtext/plainonly to the full inlinable text family, using the sharedreadAgentAttachmentText.components/ChatPageContent.tsx: chat send handler usesresolvePendingAttachments; re-exportsPendingAttachment.components/AgentCreateForm.tsxandAgentCreatePage.tsx: the new-chat create path now uses the same resolver andattachmentToContentPart, so text-family attachments are inlined on chat creation too (previously only the existing-chat path was fixed).AgentChatPage.tsx: send assembly usesattachmentToContentPart; the optimistic media-type map excludes inlined attachments so they do not render as file chips on edit.Testing
pnpm exec vitest runforchatAttachments.test.ts(33 tests) andresolvePendingAttachments.test.ts(6 tests): 39 total, covering classification, extension fallback, formatting, content-part mapping, on-demand read, read-failure fallback, restored zero-byte attachments, and upload-error skipping.pnpm lint(biome + tsc + circular deps + knip + react-compiler) clean.make pre-commitpassed..jsonfile on both an OpenAI and an Anthropic model from both the create form and an existing chat; the content reaches the model and nofile part media type application/json not supportedwarning is produced.Known limitations
string_agg(part->>'text', '')aggregation and is lossless; distinguishing inlined content from typed text would require a content-format change and is out of scope.Implementation notes and decision log
This change came out of an investigation into why a
.jsonattachment failed silently. Key findings that shaped the approach:fantasySDK; the warning is captured but never shown. The behavior is provider-agnostic, so switching models does not help.file_idand re-derives the type from the stored row. A client cannot relabel a file, so "fudge the content-type" approaches do not work and a binary cannot be smuggled in as text.text/plain,text/markdown,text/csv,application/json, all safe to inline as text.Scope decisions:
CallWarningis tracked as a follow-up to ship alongside the first reachable drop.fileId. The file is simply sent as text rather than referenced as a file part.Per-file testing follows the frontend guidelines: the new logic is pure functions covered by vitest. No Storybook story was added because the change introduces no new rendered UI and the send pipeline is not exercised by an existing story harness.
This PR was generated by Coder Agents on behalf of @johnstcn.