feat: images upload the moment you attach them - #6276
Conversation
Attachments used to ride the turn-start command as base64 data URLs: send carried the bytes, the stash re-encoded them into localStorage, and a 10MB image meant a 14M-char string in a single ws frame. Images now upload the moment they are attached. A ws RPC mints a pending-<uuid> id plus a signed, expiring upload URL (mirroring signed asset GETs, so it works against any environment); the browser POSTs the compressed bytes to it with progress and abort; the turn-start command carries id references only. The Normalizer renames pending files to their thread segment at send, resolving by uuid so retries after a partial send are idempotent. Never-sent uploads are deleted on chip removal and swept after 30 days. Breaking: the dataUrl upload variant is deleted with no compatibility path. RN mobile compiles with image attach gated off behind IMAGE_ATTACH_ENABLED and tells the user an update is needed; drafts and stash persist id references (v9/v3 storage, old payloads purged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service conventions review of the changed server/contracts code. Two findings on error modeling; the new AttachmentUpload module itself follows the repo's service/DI conventions (namespace subpath imports, dependencies acquired via yield* Foo.Foo, no hidden runtimes).
Posted via Macroscope — Effect Service Conventions
| export class AttachmentUploadRequestError extends Schema.TaggedErrorClass<AttachmentUploadRequestError>()( | ||
| "AttachmentUploadRequestError", | ||
| { | ||
| detail: Schema.String, | ||
| }, | ||
| ) { | ||
| override get message(): string { | ||
| return this.detail; | ||
| } | ||
| } |
There was a problem hiding this comment.
AttachmentUploadRequestError carries an unstructured detail string as its only data and derives message straight from it, which is the pattern this convention rules out: failures should carry stable structural attributes (e.g. attachmentId plus a normalized category, and a real cause when one exists) and derive message from those.
It is also never constructed anywhere — it only appears in the WsAttachmentsCreateUploadUrlRpc / WsAttachmentsDeleteRpc error unions and in AttachmentUploadError. Consider either giving it structural attributes at the point it will actually be raised, or dropping it from the declared error channels until a failure boundary needs it.
Posted via Macroscope — Effect Service Conventions
| const stats = yield* fileSystem.stat(claimPlan.currentPath).pipe( | ||
| Effect.mapError( | ||
| () => | ||
| new OrchestrationDispatchCommandError({ | ||
| message: `Failed to persist attachment '${attachment.name}'.`, | ||
| message: `Attachment '${attachment.name}' cannot be sent: attachment not found (removed or expired).`, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
This translation drops the underlying PlatformError, even though OrchestrationDispatchCommandError accepts an optional cause. Passing it through keeps the full error chain and stack available while the caller-visible message stays the same. Same applies to the fileSystem.rename mapping just below (line 137-144).
| const stats = yield* fileSystem.stat(claimPlan.currentPath).pipe( | |
| Effect.mapError( | |
| () => | |
| new OrchestrationDispatchCommandError({ | |
| message: `Failed to persist attachment '${attachment.name}'.`, | |
| message: `Attachment '${attachment.name}' cannot be sent: attachment not found (removed or expired).`, | |
| }), | |
| ), | |
| ); | |
| const stats = yield* fileSystem.stat(claimPlan.currentPath).pipe( | |
| Effect.mapError( | |
| (cause) => | |
| new OrchestrationDispatchCommandError({ | |
| message: `Attachment '${attachment.name}' cannot be sent: attachment not found (removed or expired).`, | |
| cause, | |
| }), | |
| ), | |
| ); |
Posted via Macroscope — Effect Service Conventions
| description: | ||
| "Browser storage is unavailable, so this stash is kept in memory only for this session.", | ||
| data: { hideCopyButton: true }, | ||
| const attachments: PersistedComposerImageAttachment[] = []; |
There was a problem hiding this comment.
🟠 High chat/ChatComposer.tsx:2095
When stashEntryToQueue returns written === false, stashCurrentPrompt aborts every non-ready attachment upload before the write and then returns early, claiming "the composer was left as-is." But the uploads are already cancelled. cancelAttachmentUpload stops the job without updating the retained image chip, so each affected chip is stuck in uploading forever, cannot be sent, and cannot be retried — the only recourse is removing and re-attaching the file. Consider moving the cancelAttachmentUpload loop (or at least its side effects) to after the written guard so a rejected stash write truly leaves the composer untouched.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/ChatComposer.tsx around line 2095:
When `stashEntryToQueue` returns `written === false`, `stashCurrentPrompt` aborts every non-ready attachment upload *before* the write and then returns early, claiming "the composer was left as-is." But the uploads are already cancelled. `cancelAttachmentUpload` stops the job without updating the retained image chip, so each affected chip is stuck in `uploading` forever, cannot be sent, and cannot be retried — the only recourse is removing and re-attaching the file. Consider moving the `cancelAttachmentUpload` loop (or at least its side effects) to after the `written` guard so a rejected stash write truly leaves the composer untouched.
| } | ||
| const extension = NodePath.extname(fileName); | ||
| const finalId = `${threadSegment}-${uuid}`; | ||
| if (currentSegment === threadSegment) { |
There was a problem hiding this comment.
🟠 High src/attachmentStore.ts:199
planAttachmentClaim returns alreadyScoped: true whenever currentSegment === threadSegment, but toSafeThreadAttachmentSegment is lossy: it case-folds, replaces punctuation, and truncates at 80 characters. Distinct thread IDs like Thread.Foo and thread-foo produce the same segment, so a turn for the second thread can reference the first thread's attachment UUID and pass as already scoped — defeating the cross-thread ownership check at line 202 and letting the wrong thread reuse another thread's file. The equality check on line 199 validates the sanitized segment, not the original thread ID, so it cannot distinguish collisions. Consider comparing the unsanitized original thread IDs (or storing an exact thread ID alongside the file) before treating a matching segment as proof of ownership.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/attachmentStore.ts around line 199:
`planAttachmentClaim` returns `alreadyScoped: true` whenever `currentSegment === threadSegment`, but `toSafeThreadAttachmentSegment` is lossy: it case-folds, replaces punctuation, and truncates at 80 characters. Distinct thread IDs like `Thread.Foo` and `thread-foo` produce the same segment, so a turn for the second thread can reference the first thread's attachment UUID and pass as already scoped — defeating the cross-thread ownership check at line 202 and letting the wrong thread reuse another thread's file. The equality check on line 199 validates the sanitized segment, not the original thread ID, so it cannot distinguish collisions. Consider comparing the unsanitized original thread IDs (or storing an exact thread ID alongside the file) before treating a matching segment as proof of ownership.
| return settledUpload ? { ...image, upload: settledUpload } : image; | ||
| }); | ||
| const turnAttachments = readyAttachmentRefs(sendableImages); | ||
| const unsentImageNames = sendableImages |
There was a problem hiding this comment.
🟡 Medium components/ChatView.tsx:5198
The "Some images were not attached" toast can fire even when the turn is never started. When updateThreadMetadata or persistThreadSettingsForNextTurn already set failure, the code still runs awaitAttachmentUploads, filters for images whose upload.status !== "ready", and shows the toast saying the message "was sent without them." But if failure !== null, startThreadTurn is skipped — no message is sent at all. The user gets a toast claiming a send happened during a failure path. Guard the toast behind failure === null so it only fires when the turn will actually be attempted.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/ChatView.tsx around line 5198:
The "Some images were not attached" toast can fire even when the turn is never started. When `updateThreadMetadata` or `persistThreadSettingsForNextTurn` already set `failure`, the code still runs `awaitAttachmentUploads`, filters for images whose `upload.status !== "ready"`, and shows the toast saying the message "was sent without them." But if `failure !== null`, `startThreadTurn` is skipped — no message is sent at all. The user gets a toast claiming a send happened during a failure path. Guard the toast behind `failure === null` so it only fires when the turn will actually be attempted.
| * it needs the settled states, because the draft it read them from is already | ||
| * cleared by then. | ||
| */ | ||
| export async function awaitAttachmentUploads( |
There was a problem hiding this comment.
🟠 High lib/attachmentUploadQueue.ts:266
awaitAttachmentUploads drops successfully uploaded images from the sent message. It looks up jobs in jobsByImageId to await their settlement, but finishJob deletes a job from that map as soon as it settles. If an upload completes before awaitAttachmentUploads runs, the job is gone, the image is omitted from the returned map, and the caller falls back to a stale uploading snapshot — silently losing a ready attachment from the message. The caller should read the current upload state from the store for any image not still in jobsByImageId, rather than skipping it.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/lib/attachmentUploadQueue.ts around line 266:
`awaitAttachmentUploads` drops successfully uploaded images from the sent message. It looks up jobs in `jobsByImageId` to await their settlement, but `finishJob` deletes a job from that map as soon as it settles. If an upload completes before `awaitAttachmentUploads` runs, the job is gone, the image is omitted from the returned map, and the caller falls back to a stale `uploading` snapshot — silently losing a ready attachment from the message. The caller should read the current upload state from the store for any image not still in `jobsByImageId`, rather than skipping it.
| imageCount: summarizeAttachmentUploads(composerImages).ready, | ||
| terminalContexts: composerTerminalContexts, | ||
| elementContextCount: | ||
| composerElementContexts.length + |
There was a problem hiding this comment.
🟠 High components/ChatView.tsx:4953
Sending an annotation via the direct pick-and-send path with an otherwise empty composer silently does nothing while the image is still uploading. hasSendableContent is computed from composerPreviewAnnotations.length, which uses the value before the direct annotation is appended, and imageCount now counts only uploaded images. With no draft text and the annotation image still in flight, both are zero, so onSend returns at the empty-content guard and never reaches the attachment-upload wait. The composerPreviewAnnotations variable built later already includes the annotation — it should be used for the element-context count instead of the pre-append composerPreviewAnnotations.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/ChatView.tsx around line 4953:
Sending an annotation via the direct pick-and-send path with an otherwise empty composer silently does nothing while the image is still uploading. `hasSendableContent` is computed from `composerPreviewAnnotations.length`, which uses the value *before* the direct annotation is appended, and `imageCount` now counts only uploaded images. With no draft text and the annotation image still in flight, both are zero, so `onSend` returns at the empty-content guard and never reaches the attachment-upload wait. The `composerPreviewAnnotations` variable built later already includes the annotation — it should be used for the element-context count instead of the pre-append `composerPreviewAnnotations`.
There was a problem hiding this comment.
🟡 Medium
t3code/apps/web/src/composerDraftStore.ts
Line 3253 in 1331956
removePreviewAnnotation filters the annotation's image out of images but never calls releaseComposerAttachment or cancelAttachmentUpload. Removing a preview annotation while its image is still uploading leaves the upload job running, and removing one after upload completes leaves the server-side pending attachment allocated until the 30-day backstop sweep. The image disappears from the draft's images array, so the upload queue never sees it again to release or cancel it.
Also found in 1 other location(s)
apps/web/src/components/chat/ChatComposer.tsx:1510
The environment-retargeting path calls
retryAttachmentUploadfor an already-readyimage but never releases its existing server attachment.retryAttachmentUploadonly cancels an active job (there is none after readiness) and starts a new upload, whereasreleaseComposerAttachmentis the function that deletes a ready attachment. Switching a draft between environments therefore leaves one orphaned pending upload behind on each retarget until the 30-day sweep.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/composerDraftStore.ts around line 3253:
`removePreviewAnnotation` filters the annotation's image out of `images` but never calls `releaseComposerAttachment` or `cancelAttachmentUpload`. Removing a preview annotation while its image is still uploading leaves the upload job running, and removing one after upload completes leaves the server-side pending attachment allocated until the 30-day backstop sweep. The image disappears from the draft's `images` array, so the upload queue never sees it again to release or cancel it.
Also found in 1 other location(s):
- apps/web/src/components/chat/ChatComposer.tsx:1510 -- The environment-retargeting path calls `retryAttachmentUpload` for an already-`ready` image but never releases its existing server attachment. `retryAttachmentUpload` only cancels an active job (there is none after readiness) and starts a new upload, whereas `releaseComposerAttachment` is the function that deletes a ready attachment. Switching a draft between environments therefore leaves one orphaned pending upload behind on each retarget until the 30-day sweep.
| // different one, an attachment we still hold the File for is silently | ||
| // re-uploaded; one restored after a reload has no File to re-send, so it | ||
| // fails and has to be removed. | ||
| useEffect(() => { |
There was a problem hiding this comment.
🟠 High chat/ChatComposer.tsx:1506
For a reload-restored attachment whose bytes exist in environment A, switching the draft to environment B overwrites the ready upload state (including attachmentId and the original environmentId) with a generic failed state. Switching the draft back to environment A does not recover the attachment — resolveAttachmentEnvironmentAction preserves any non-ready state, so the attachment stays permanently failed even though its bytes are still available in environment A. The user must remove and reattach it. Consider preserving the original ready snapshot and deriving availability from the stored environmentId instead of clobbering the upload state, so switching back to the original environment restores the attachment.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/ChatComposer.tsx around line 1506:
For a reload-restored attachment whose bytes exist in environment A, switching the draft to environment B overwrites the `ready` upload state (including `attachmentId` and the original `environmentId`) with a generic `failed` state. Switching the draft back to environment A does not recover the attachment — `resolveAttachmentEnvironmentAction` preserves any non-ready state, so the attachment stays permanently failed even though its bytes are still available in environment A. The user must remove and reattach it. Consider preserving the original `ready` snapshot and deriving availability from the stored `environmentId` instead of clobbering the upload state, so switching back to the original environment restores the attachment.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1331956. Configure here.
| "Browser storage rejected the write, so the composer was left as-is. Free up site data and try again.", | ||
| data: { hideCopyButton: true }, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
Stash cancel leaves chips stuck
High Severity
stashCurrentPrompt cancels in-flight uploads before the stash write is confirmed. On a quota/written: false failure the composer is left unchanged, but cancelAttachmentUpload never flips chip state off uploading, and the job is already gone. Send stays blocked on “still uploading” with no retry control.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1331956. Configure here.
| role: "user", | ||
| text: outgoingMessageText, | ||
| attachments: turnAttachmentsResult.value, | ||
| attachments: turnAttachments, |
There was a problem hiding this comment.
Retry restores stale upload state
Medium Severity
After awaitAttachmentUploads, sendable images are built from settled results, but the failure restore path rehydrates composerImagesSnapshot instead. Pick-and-send can finish uploads during the await; on turn-start failure those chips come back as uploading with no live job, so send stays blocked.
Reviewed by Cursor Bugbot for commit 1331956. Configure here.
| mimeType, | ||
| sizeBytes, | ||
| dataUrl, | ||
| environmentId, |
There was a problem hiding this comment.
Discard skips pending upload cleanup
Medium Severity
Draft discard and preview-annotation removal drop composer images without releaseComposerAttachment. In-flight uploads keep running and ready pending-* bytes stay on the server, contrary to the upload-on-attach cleanup contract.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 1331956. Configure here.
| }): void { | ||
| cancelAttachmentUpload(input.image.id); | ||
| startAttachmentUpload(input); | ||
| } |
There was a problem hiding this comment.
Env reupload leaks old bytes
Medium Severity
Environment retargeting calls retryAttachmentUpload for a ready attachment on another environment, but retry only cancels an in-flight job and starts a new upload. The previous environment’s ready pending-* object is never deleted.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1331956. Configure here.
| } | ||
| if (result.error) { | ||
| setPendingConnectionError(result.error); | ||
| } |
There was a problem hiding this comment.
Mobile paste fails silently
Medium Severity
With IMAGE_ATTACH_ENABLED false, native paste converts to no attachments and only console.warns. Unlike the picker path, paste never calls setPendingConnectionError, so users get no “needs an app update” banner when a paste is dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1331956. Configure here.
ApprovabilityVerdict: Needs human review 7 blocking correctness issues found. This PR introduces upload-on-attach for image attachments — a significant new feature that adds server upload infrastructure, new HTTP routes, client upload queue management, and contract changes. Multiple HIGH severity findings identify potential issues in attachment ownership validation, upload state handling, and cleanup flows that should be addressed before merge. You can customize Macroscope's approvability policy. Learn more. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
1 participant
Attached images used to ride inside the send command as base64 data URLs. Sending a message with a 10MB screenshot meant a 14M-character string in one ws frame: send felt slow, the turn could not start until the frame landed, and the stash had to re-encode every image to squeeze it into localStorage.
Now the bytes move while you are still typing. Attaching an image mints a
pending-<uuid>id plus a signed upload URL over ws (the same pattern as signed asset GETs, so it works against any environment), and the browser POSTs the compressed bytes to it with per-chip progress and cancel. The send command carries id references only, and the server renames pending files to their thread at turn start — resolving by uuid, so send retries and existing asset URLs survive the rename.What this buys:
.partthen rename, so a partial transfer is never a valid attachment, and the route enforces the exact minted byte count.Breaking, deliberately without a compatibility path: the dataUrl upload variant is deleted. Out-of-date clients get a loud schema error. React Native mobile compiles and degrades honestly behind
IMAGE_ATTACH_ENABLED = false("Image attach needs an app update"); the port is a fast-follow.apps/swift-iosis not on main, so the Swift mirror is untouched — a 7-edit patch list for that branch is in the plan doc.Design doc: https://rztz3kvilrh0.postplan.dev
Verified: contracts 227, server attachment suites 66 (full suite green except 6 launcher-version failures that reproduce on untouched main), web 2201, mobile 654. Lint and format clean. Not yet done: a manual browser pass (paste several images fast, cancel mid-flight, kill the network, reload with a ready draft) — automated browsers cannot see authenticated views here.
Built by Claude Fable 5 running in Claude Code.
🤖 Generated with Claude Code
Note
High Risk
Breaking contract change to attachment handling plus new signed upload auth/storage paths in a security- and data-sensitive area; mobile image attach is temporarily disabled.
Overview
Replaces data-URL-in-send with upload-on-attach. Attached images now mint a
pending-<uuid>id and signed upload URL over ws, POST bytes over HTTP with per-chip progress, and ridethread.turn.startas id references only. The server claims pending files into the thread at turn start (uuid-stable, so retries and asset URLs survive the rename).Web composer blocks send while any chip is uploading or failed, persists only ready server ids (draft storage v9), and restores previews via signed asset URLs. Stash no longer re-encodes images into localStorage. Never-sent uploads are deleted on chip removal, with a 30-day pending/
*.partsweep as backstop.Mobile disables image attach behind
IMAGE_ATTACH_ENABLED = falseand warns when legacy draft/outbox images cannot be sent. The dataUrl upload path is removed with no compatibility shim.Reviewed by Cursor Bugbot for commit 1331956. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Upload image attachments immediately on attach instead of encoding them inline at send time
issueAttachmentUploadUrl), a POST HTTP route for receiving bytes (attachmentUploadRouteLayer), atomic.part-file writes, and pending-to-thread-scoped file claiming in the normalizer.attachmentsCreateUploadUrl,attachmentsDelete) and corresponding auth scope requirements.dataUrlpayloads are dropped on rehydration.IMAGE_ATTACH_ENABLED = falsepending a mobile upload-on-attach implementation; legacy queued attachments are sent without bytes and a warning is shown.📊 Macroscope summarized 1331956. 29 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.