feat(frontend): attachment transport, references, and rendering for the agent chat - #5619
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAgent attachment flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AgentConversation
participant AttachmentAPI
participant AgentMessage
participant SessionContentAPI
User->>AgentConversation: stage and submit attachment
AgentConversation->>AttachmentAPI: upload multipart attachment
AttachmentAPI-->>AgentConversation: attachment metadata
AgentConversation->>AgentMessage: render session-scoped file part
AgentMessage->>SessionContentAPI: fetch attachment content
SessionContentAPI-->>AgentMessage: authenticated media blob
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ccbb31f to
153f34e
Compare
dosubot
Bot
added
size:XXL
Railway Preview Environment
Updated at 2026-08-01T18:15:23.131Z |
e760f6c to
42d83fe
Compare
a039890 to
db458d6
Compare
42d83fe to
df5321b
Compare
db458d6 to
aa1cb47
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
web/oss/vitest.config.ts (2)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a glob instead of a hard-coded file list.
test.includelists six specific test files. A new*.test.tsfile added underAgentChatSlicewill not run underpnpm test:unitunless someone remembers to add it to this list.Use a glob such as
"src/components/AgentChatSlice/**/*.test.ts"to pick up new test files automatically.♻️ Proposed refactor to use a glob pattern
include: [ - "src/components/AgentChatSlice/assets/attachments.test.ts", - "src/components/AgentChatSlice/assets/attachmentTransport.test.ts", - "src/components/AgentChatSlice/assets/files.test.ts", - "src/components/AgentChatSlice/assets/inFlightSubmit.test.ts", - "src/components/AgentChatSlice/assets/transcriptToMessages.test.ts", - "src/components/AgentChatSlice/hooks/useAttachmentUploads.test.ts", + "src/components/AgentChatSlice/**/*.test.ts", ],
5-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
test.includepatterned to include new AgentChatSlice tests.This hard-coded list now covers six files; a new
*.{test,spec}.{ts,tsx}file added under this component will not run unless it is manually appended here. Use a glob such as"src/components/AgentChatSlice/**/*.{test,spec}.{ts,tsx}"so new tests are included automatically.web/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.test.ts (1)
1-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the hook's stateful upload lifecycle.
This file tests only
removeUploadFileandisUploadRetryReady. The hook'srun,retry,enqueue, and the remount-resume effect inuseAttachmentUploads.tscarry the real complexity (controller replacement, retry-timer scheduling, and resume-on-remount), but none of that is exercised here.Add
renderHook-based tests for:
- A successful upload transitioning a file from
uploadingtodone.- A failed upload with
retryAfterSecondsblockingretryuntil the deadline, then auto-running.abortcancelling an in-flight upload and clearing its retry timer.- The remount-resume effect requeuing a file left in
uploadingstatus with no live controller.Do you want help drafting these tests?
web/oss/src/components/AgentChatSlice/AgentConversation.tsx (1)
1945-1956: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive the extra-file uploads an abortable controller.
Each extra upload receives the signal of a discarded
AbortController, so nothing can cancel it. The conversation can unmount (session switch, pane close) while the voice clip is still uploading, and the request continues. The staged-file path avoids this:useAttachmentUploadskeeps a controller per uid and aborts all of them on unmount.Consider staging the extra entries through
setFilesplusuploads.enqueueso extras share the abort, progress, and retry handling of the tray, or keep a component-scoped controller for these direct uploads.web/oss/src/components/AgentChatSlice/components/AgentMessage.tsx (1)
261-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDownload through the shared axios helper instead of raw
fetch.
fetch(src, {credentials: "include"})sends cookies only. The attachment endpoint is called everywhere else through the shared axios instance, which attaches the app's auth headers (seefetchAttachmentBlobinweb/oss/src/components/AgentChatSlice/assets/attachmentMedia.ts). So this direct request fails on every deployment that authenticates by header, and each download pays a failed round trip before the fallback state machine starts.Call
fetchAttachmentBlob({sessionId, attachmentId})directly here. The handler then needs nosource.onError()mode switch and nofallbackDownloadPendingeffect, which also removes the state coupling at lines 251-259.web/oss/src/components/AgentChatSlice/assets/attachmentMedia.ts (1)
48-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
useAttachmentObjectUrlhook.
useAttachmentMediaSrcalready handles the blob object URL case, and no tracked file imports or callsuseAttachmentObjectUrl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d160b74-1259-42a5-bbd8-948376474ff7
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
docs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.mddocs/design/agent-workflows/projects/agent-multi-modality/status.mdhosting/docker-compose/oss/env.oss.dev.exampleweb/oss/package.jsonweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/assets/attachmentMedia.tsweb/oss/src/components/AgentChatSlice/assets/attachmentTransport.test.tsweb/oss/src/components/AgentChatSlice/assets/attachmentTransport.tsweb/oss/src/components/AgentChatSlice/assets/attachments.test.tsweb/oss/src/components/AgentChatSlice/assets/attachments.tsweb/oss/src/components/AgentChatSlice/assets/constants.tsweb/oss/src/components/AgentChatSlice/assets/files.test.tsweb/oss/src/components/AgentChatSlice/assets/files.tsweb/oss/src/components/AgentChatSlice/assets/inFlightSubmit.test.tsweb/oss/src/components/AgentChatSlice/assets/inFlightSubmit.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.tsweb/oss/src/components/AgentChatSlice/components/AgentMessage.tsxweb/oss/src/components/AgentChatSlice/components/AudioPlayer.tsxweb/oss/src/components/AgentChatSlice/components/ComposerAttachments.tsxweb/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.test.tsweb/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.tsweb/oss/test-results/junit.xmlweb/oss/tests/playwright/acceptance/agent-chat/attach-send-render-reload.spec.tsweb/oss/vitest.config.tsweb/packages/agenta-ui/src/RichChatInput/RichChatInput.tsxweb/packages/agenta-ui/src/RichChatInput/plugins/SendButton.tsxweb/packages/agenta-ui/src/RichChatInput/plugins/SubmitPlugin.tsx
| 1. **The in-message delivery notice was removed** ("<filename>: the model did not perceive this | ||
| file. The agent can use it at attachments/…"). Its wording and placement were | ||
| implementation-invented. | ||
| 2. **The chip-level hint was removed too** (the crossed-eye indicator with "The model may not | ||
| perceive this file…"). The first release therefore ships no notice UI at all; decision D6's | ||
| "visible notice" is deferred to a future surface the product owner designs. The | ||
| `attachment_delivery` events keep flowing and persisting unchanged, and the front end parses | ||
| and ignores them, so that future surface needs no wire changes. The upload error messages | ||
| were reviewed and kept. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the notice behavior in status.md.
This section states that the first release ships no notice UI. However, status.md still describes a visible notice at Line [19]–[20] and repeats the D6 notice at Line [52]–[60]. Update those statements before merging so the release records define one user-visible behavior.
| | 0 | Close the silent-failure gap: gate the ungated paste and drag path on `NEXT_PUBLIC_AGENT_FILE_UPLOADS` | in review (PR #5604) | | ||
| | 1 | First user-visible release: the attachment resource and storage, the record-schema extension, the runner's resolve-materialize-and-deliver seam for images, structured capability errors, the minimum security and limits work (per the settled matrix in [design.md](design.md), including the gateway raise to 32 MB), and the front-end transport and reference wiring | in review as four stacked PRs (#5607 API, #5615 runner, #5617 SDK, #5619 front end), end-to-end QA green on the dev stack; the trail is in [protocols/stage-1.md](protocols/stage-1.md) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the opening status with the stage tracker.
The file still says that implementation is the next step and that the backend has not started. The tracker now says Stage 1 is in review with end-to-end QA complete. Update the stale opening text so this page has one current lifecycle state.
| - Review and merge the Stage 1 train bottom-up (#5604, #5607, #5598, #5615, #5597, #5617, | ||
| #5619), then flip `NEXT_PUBLIC_AGENT_FILE_UPLOADS` in production as the rollout's fifth act. | ||
| - Follow-ups recorded in [protocols/stage-1.md](protocols/stage-1.md): the CI report glob line | ||
| (needs a workflow-scoped push), the Fern client regeneration, and the zip-container | ||
| classifier refinement. | ||
| - Then Stage 2 of [plan.md](plan.md). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unrelated PR from the Stage 1 merge sequence.
protocols/stage-1.md records PR #5598 as pre-existing and unrelated at Line [388]–[390], but this checklist includes it in the Stage 1 train. Remove it or document the attachment dependency before using this list as the release sequence.
| const url = useMemo(() => (blob ? URL.createObjectURL(blob) : null), [blob]) | ||
| useEffect(() => { | ||
| return () => { | ||
| if (url) URL.revokeObjectURL(url) | ||
| } | ||
| }, [url]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Create the object URL in an effect, not in useMemo.
useMemo is not a guaranteed cache, and its result is not tied to the effect lifecycle. Two consequences apply here, and to the identical pattern at lines 85-90:
- Under React StrictMode the effect runs mount → unmount → mount. The cleanup revokes the URL, then the second mount re-registers cleanup for the same memoized string. The
srcthen points at a revoked URL and the media fails to load. - If React drops the memoized value without the effect running, the created URL is never revoked.
Derive the URL inside the effect and keep it in state instead.
♻️ Proposed refactor
- const url = useMemo(() => (blob ? URL.createObjectURL(blob) : null), [blob])
- useEffect(() => {
- return () => {
- if (url) URL.revokeObjectURL(url)
- }
- }, [url])
+ const [url, setUrl] = useState<string | null>(null)
+ useEffect(() => {
+ if (!blob) {
+ setUrl(null)
+ return
+ }
+ const next = URL.createObjectURL(blob)
+ setUrl(next)
+ return () => {
+ setUrl(null)
+ URL.revokeObjectURL(next)
+ }
+ }, [blob])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const url = useMemo(() => (blob ? URL.createObjectURL(blob) : null), [blob]) | |
| useEffect(() => { | |
| return () => { | |
| if (url) URL.revokeObjectURL(url) | |
| } | |
| }, [url]) | |
| const [url, setUrl] = useState<string | null>(null) | |
| useEffect(() => { | |
| if (!blob) { | |
| setUrl(null) | |
| return | |
| } | |
| const next = URL.createObjectURL(blob) | |
| setUrl(next) | |
| return () => { | |
| setUrl(null) | |
| URL.revokeObjectURL(next) | |
| } | |
| }, [blob]) |
df5321b to
d9f9f93
Compare
927cb78 to
9e02eef
Compare
…he agent chat
Attaching a file now uploads it once through the create route (axios with
progress, the documented Fern exception; the tray uid doubles as the
idempotency key so retry reuses the original identity) and the outgoing
message carries a reference: the real content URL in the part's url and
{attachmentId, size} in providerMetadata.agenta, never base64. Upload errors
are told apart (cap 413, invalid 422, quota 429, in-flight 409 honoring
Retry-After, old-backend 404); removing a chip aborts its request; send blocks
with a tooltip until every upload settles, and a double-send guard covers the
voice path's upload await. Voice recordings use references only when uploads
are enabled and keep the inline path otherwise, so the two flags stay
independent. Perception derives from the model's modalities (absent or
unknown means the workspace-only courtesy notice), the same memoized fact the
voice controls read. Sent and reloaded conversations render attachments
through the content route with authenticated blob fallback and no eager
downloads; durable records rebuild user file parts with filenames; delivery
events render as named per-file notices in live and replay. web/oss gains a
vitest setup with an explicit include list and junit reporting; the CI glob
addition for its report is left uncommitted because the push credential lacks
the workflow scope, recorded in the protocol. Dev stacks default the uploads
flag on; production flips separately as the rollout's fifth act.
Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…status The status update rides the top lane deliberately: committing it on the docs lane would rebase the whole train for a bookkeeping edit. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
The rendered sentence under the answer was implementation-invented wording; the product owner rejected it. D6's visible notice is the chip's crossed-eye hint alone. The attachment_delivery events keep flowing and persisting; the front end parses and ignores them, so a future surface can render them without wire changes. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
The previous commit's staging dropped this file's hunk; without it a replayed delivery record still produced a rendered part. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
The crossed-eye workspace-only hint goes the way of the in-message notice: the first release ships no notice UI, and decision D6's visible notice waits for a surface the product owner designs. The perception plumbing that existed only to feed the hint goes with it; the voice controls keep their own modality read. The per-turn count rises from the dark-shipped 5 to 100. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…anup to captured files
d9f9f93 to
caf7ab2
Compare
92797ad to
c7ac4f6
Compare
1 participant
The composer collects files but has no transport: every attachment inlines as a base64 data URL, bloats browser storage and the wire, disappears from reloaded sessions, and the person never learns whether the model actually saw it. This is work package 4 of 4 of the Stage 1 plan, the producer the reader-first rollout saved for last, stacked on the SDK package #5617.
Before: attach inlines base64; a reloaded session loses user attachments; no upload progress, no retry, no honest errors; nothing tells the user a file went workspace-only.
After: attaching uploads once through
POST /sessions/attachments(progress, abort on chip removal, retry reusing the same idempotency key, send blocked with a tooltip until every upload settles) and the message carries a reference: the content URL in the part plus{attachmentId, size}inproviderMetadata.agenta, never base64. Upload errors are told apart (cap, invalid file, session quota, in-flight with Retry-After, old-backend 404). Sent and reloaded conversations render through the content route with an authenticated blob fallback and no eager downloads; durable records rebuild user file parts with filenames;attachment_deliveryevents are parsed and persisted but render nothing (the product owner removed both notice surfaces; a future surface can render the persisted events without wire changes). The per-turn attachment count is 100 (raised from the dark-shipped 5 at the product owner's direction; the per-session quotas are the real bound). Voice recordings use references only when the uploads flag is on and keep their inline path otherwise, so the two flags stay independent.Adversarially reviewed before opening: fourteen findings, four merge blockers, all fixed. The blockers were exactly the quiet kind: the acceptance Playwright spec sat one directory outside the collected test root (it now lives with its siblings, tagged, no silent skip); voice messages would have broken whenever uploads were off;
sizesat top-level where the AI SDK's validation strips it; and a hardcoded perception map contradicted the capability data computed ten lines away. Full trail inprotocols/stage-1.md.Implicit decisions and their tradeoffs
RichChatInput,SubmitPlugin,SendButtongain send-only disabling). The review enumerated all three consumers; the new props default to today's behavior, so the other two surfaces are untouched.generateId()is used overcrypto.randomUUIDbecause dev boxes are reached over plain HTTP where the latter is undefined.DEFAULT_ATTACHMENT_LIMITS): capability-derived limits are a Stage 2 item.Forced routes to double-check
QA
lint clean, typecheck clean, 34 unit tests green with junit output. The full end-to-end live QA ran on the dev stack with all four packages deployed (full record in
protocols/stage-1.md):providerMetadata.agenta.attachmentId, zero base64 in 2.3 KB of body), and the model reads the image's text verbatim.{}and every upload 422'd). Fixed in this diff with a comment stating the trap.https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx