Add attachment support to email_reply outbound sends - #701
Merged
Merged
Conversation
Attachments (filename, content_type, content_base64; up to 10) ride the reply through both provider paths: raw bytes via the EMAIL binding and base64 via the REST fallback. Bytes are stored per-attachment in R2 (storage_kind 'external') so email_attachment_get serves outbound attachments, with metadata-only degradation when the blob put fails. With attachments the whole message is gated by the plan's email_message_bytes cap; storage-bytes estimation and email_send usage bytes include attachment payloads. Retention pruning, message deletion, and account deletion now clean up external attachment blobs. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughAdds optional outbound email attachments with validation, R2-backed external storage, provider delivery, retrieval, usage accounting, retention cleanup, account deletion support, and updated reply and architecture documentation. ChangesOutbound email attachments
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
kody-bot
marked this pull request as ready for review
July 10, 2026 05:28
Contributor
|
🔎 Preview deployed: https://kody-pr-701.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/worker/src/app/account-deletion.ts (2)
377-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
getErrorMessagefor consistency with sibling catch blocks.This catch block uses
error instanceof Error ? error.message : String(error)while every other handler in the samePromise.allusesgetErrorMessage(error). Aligning the pattern improves consistency and ensures any extra error-normalization logic ingetErrorMessageapplies here too.♻️ Proposed refactor
listUserEmailBlobKeys(input.env, input.userId).catch((error) => { - const message = error instanceof Error ? error.message : String(error) + const message = getErrorMessage(error) input.warnings.push(`Failed to enumerate email blob keys: ${message}`) return [] as Array<string> }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/account-deletion.ts` around lines 377 - 379, Replace the inline error conversion in the catch callback for listUserEmailBlobKeys with getErrorMessage(error), matching the other Promise.all handlers and reusing the existing normalization helper.
286-307: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider parallelizing the two independent D1 queries in
listUserEmailBlobKeys.The raw-MIME and attachment-key queries are independent and could run concurrently via
Promise.all, halving the latency of this inventory step.♻️ Proposed refactor
async function listUserEmailBlobKeys(env: Env, userId: string) { - const rawMimeRows = await env.APP_DB.prepare( + const [rawMimeRows, attachmentRows] = await Promise.all([ + env.APP_DB.prepare( `SELECT raw_mime_key FROM email_messages WHERE user_id = ? AND raw_mime_key IS NOT NULL`, ) .bind(userId) - .all<{ raw_mime_key: string }>() - // Externally stored attachments (outbound mail) have their own R2 - // objects, separate from any raw-MIME blob. - const attachmentRows = await env.APP_DB.prepare( + .all<{ raw_mime_key: string }>(), + // Externally stored attachments (outbound mail) have their own R2 + // objects, separate from any raw-MIME blob. + env.APP_DB.prepare( `SELECT attachment.storage_key AS storage_key FROM email_attachments attachment JOIN email_messages message ON message.id = attachment.message_id WHERE message.user_id = ? AND attachment.storage_key IS NOT NULL`, ) .bind(userId) - .all<{ storage_key: string }>() + .all<{ storage_key: string }>(), + ]) return uniqueStrings([ ...(rawMimeRows.results ?? []).map((row) => row.raw_mime_key), ...(attachmentRows.results ?? []).map((row) => row.storage_key), ]) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/account-deletion.ts` around lines 286 - 307, Parallelize the independent raw-MIME and attachment-key queries in listUserEmailBlobKeys by creating both D1 query promises before awaiting them, then await them together with Promise.all and preserve the existing result mapping and uniqueStrings behavior.packages/worker/src/app/retention.ts (2)
813-817: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
deletedAttachmentBlobscalculation is indirect and fragile.
blobKeys.length - result.deletedRawMimeBlobsworks only because each row contributes at most oneraw_mime_key. If the data model ever allows multiple raw-MIME keys per row, this subtraction would produce incorrect attachment counts. Computing the attachment count explicitly from the map is more robust.♻️ Proposed explicit calculation
try { await input.blobs.delete(blobKeys) result.deletedRawMimeBlobs = rowsWithBlob.filter( (row) => row.raw_mime_key !== null, ).length - result.deletedAttachmentBlobs = - blobKeys.length - result.deletedRawMimeBlobs + result.deletedAttachmentBlobs = rowsWithBlob.reduce( + (count, row) => + count + (attachmentKeysByMessageId.get(row.id)?.length ?? 0), + 0, + ) } catch (error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/retention.ts` around lines 813 - 817, In the retention result calculation, replace the indirect `blobKeys.length - result.deletedRawMimeBlobs` assignment with an explicit count of attachment blobs derived from `blobKeys` or the associated map, excluding entries representing `raw_mime_key`; update `result.deletedAttachmentBlobs` directly so the count remains correct if a row can contain multiple raw-MIME keys.
779-779: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local placeholder string
packages/worker/src/app/retention.ts:779Theconst placeholdersin the attachment loop shadows the module-levelplaceholders(values)helper used later in the file. Rename it to something likechunkPlaceholdersto avoid the overlap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/retention.ts` at line 779, The attachment loop’s local placeholders string conflicts with the module-level placeholders(values) helper. In the attachment loop, rename the local const placeholders variable to chunkPlaceholders and update its usages in that loop.packages/worker/src/email/outbound.ts (1)
339-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
contentId: undefinedis redundant in the REST payload.
JSON.stringifyomits properties withundefinedvalues, socontentId: undefinedhas no effect on the serialized request body. It can be removed for clarity, or if the REST API actually expectscontentIdto be absent for attachments, a comment explaining the omission would suffice.♻️ Proposed cleanup
? // The REST API expects base64 string content. input.attachments.map((attachment) => ({ content: attachment.contentBase64, filename: attachment.filename, type: attachment.contentType, disposition: 'attachment' as const, - contentId: undefined, })) : undefined,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/email/outbound.ts` around lines 339 - 349, Remove the redundant contentId: undefined property from the attachment objects created in the outbound email payload mapping, preserving the existing fields and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/email/outbound.ts`:
- Around line 202-252: storeOutboundAttachments can orphan successfully uploaded
R2 blobs when insertEmailAttachments fails. Track the storage keys for
attachments where the blob put succeeded, wrap the insertEmailAttachments call
in a try/catch, delete those keys from input.blobs on failure, then rethrow the
original database error.
In `@packages/worker/src/email/repo.ts`:
- Around line 903-939: Replace the separate D1 delete calls in the message
cleanup function with a single input.db.batch() containing the email_attachments
and email_messages DELETE statements, preserving their order and bindings so
both deletes commit or roll back together before the R2 deletion loop.
---
Nitpick comments:
In `@packages/worker/src/app/account-deletion.ts`:
- Around line 377-379: Replace the inline error conversion in the catch callback
for listUserEmailBlobKeys with getErrorMessage(error), matching the other
Promise.all handlers and reusing the existing normalization helper.
- Around line 286-307: Parallelize the independent raw-MIME and attachment-key
queries in listUserEmailBlobKeys by creating both D1 query promises before
awaiting them, then await them together with Promise.all and preserve the
existing result mapping and uniqueStrings behavior.
In `@packages/worker/src/app/retention.ts`:
- Around line 813-817: In the retention result calculation, replace the indirect
`blobKeys.length - result.deletedRawMimeBlobs` assignment with an explicit count
of attachment blobs derived from `blobKeys` or the associated map, excluding
entries representing `raw_mime_key`; update `result.deletedAttachmentBlobs`
directly so the count remains correct if a row can contain multiple raw-MIME
keys.
- Line 779: The attachment loop’s local placeholders string conflicts with the
module-level placeholders(values) helper. In the attachment loop, rename the
local const placeholders variable to chunkPlaceholders and update its usages in
that loop.
In `@packages/worker/src/email/outbound.ts`:
- Around line 339-349: Remove the redundant contentId: undefined property from
the attachment objects created in the outbound email payload mapping, preserving
the existing fields and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 342c73d0-b65c-458b-80a4-a0b7e15567f8
📒 Files selected for processing (12)
docs/contributing/architecture/primitives.yamldocs/use/email-primitives.mdpackages/shared/src/outbound-email.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/email/cloudflare-email.tspackages/worker/src/app/retention.node.test.tspackages/worker/src/app/retention.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/repo.tspackages/worker/src/mcp/capabilities/email/email-reply.node.test.tspackages/worker/src/mcp/capabilities/email/email-reply.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…h attachment inserts Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 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 278b484. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
email_reply(and theemail.replyruntime helper that delegates to it) now accepts an optionalattachmentsarray — up to 10 of{ filename, content_type, content_base64 }— and sends the files with the reply.How
sendOutboundEmailvalidates and decodes attachments up front (count cap, base64 validity, non-empty), passes raw bytes to theEMAILbinding and base64 to the Cloudflare REST fallback (both provider paths support attachments natively).EMAIL_BLOBSunder a new key contractemail-attachment:v1:{userId}/{messageId}/{attachmentId}withemail_attachmentsrows usingstorage_kind: 'external', soemail_attachment_getserves outbound attachments through a new external-storage read branch ingetEmailAttachmentById. A failed blob put degrades that attachment to metadata-only (storage_kind: 'unavailable') instead of blocking the send.email_message_bytesper-message cap (fallback backstop for plan-less users); body-only sends keep their existing behavior. Storage-bytes estimation andemail_sendusage bytes now include attachment payloads.deleteEmailMessageById, hourly retention pruning, and account deletion all clean up external attachment blobs (account deletion's email blob enumeration now includes attachment storage keys).Testing
email-reply.node.test.ts: attachments pass through the capability tosendOutboundEmail.outbound.workers.test.ts: binding receives raw bytes and the stored attachment round-trips throughgetEmailAttachmentById; REST fallback receives base64 attachments; invalid base64 and oversize attachments are rejected before consuming daily send quota; message deletion removes the external blob.retention.node.test.ts: prune deletes external attachment blobs alongside raw-MIME blobs.npm run validategreen locally (800 unit tests, Playwright E2E, MCP E2E).contentIdkey for the schema type; attachment persistence failures mark the stored messagefailed(instead of leaving it instoredlimbo); attachment row inserts are batched (chunked at 50); a 5 MiB combined hard cap is enforced from base64 lengths before decoding; retention blob deletes are chunked below the R2 1000-key bulk limit.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@e5a88469· Head:231e249cClassification: extends — the email send contract and the email blob storage key contract both gain attachment support; no new primitives.
Primitives touched
emailemail_replyinput gainsattachments; outbound pipeline stores and sends thememail-blobs-r2email-attachment:v1:{userId}/{messageId}/{attachmentId}key contractentitlementsemail_message_bytes/storage_bytesusage-meteringemail_sendusage bytes include attachment payloadsretentionaccount-deletionSystem map
A reply with attachments flows from the
email_replycapability through the outbound pipeline into R2 blob storage and out via the Cloudflare email provider; retention and account deletion clean the new blobs up.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
Invariants
Per-user isolation holds: the new R2 key contract embeds
userId, the external read path stays behind the userId-scopedemail_attachmentsjoin, and account deletion / retention enumerate and delete the new blobs by stored keys.Summary by CodeRabbit