Skip to content

feat(email): allow sending actual attachments in emails#935

Merged
dev-rb merged 55 commits intomainfrom
rahul/comms-103-ui-for-sending-actual-attachments-in-email
Jan 14, 2026
Merged

feat(email): allow sending actual attachments in emails#935
dev-rb merged 55 commits intomainfrom
rahul/comms-103-ui-for-sending-actual-attachments-in-email

Conversation

@dev-rb
Copy link
Copy Markdown
Contributor

@dev-rb dev-rb commented Jan 12, 2026

Summary

This PR adds the ability to send actual normal email attachments instead of macro attachments when sending messages in a thread. The same functionality for email compose will be done in a separate PR since we don't currently have it setup to save drafts of composed messages

Also:

  • Updates AttachMenu for emails to use DropdownMenu
  • Changes EmailAttachmentPill to not rely on Attachment type
  • Adds a plural string util
  • Generates types

Screenshots, GIFs, and Videos

dev-rb added 30 commits January 2, 2026 12:32
…3-ui-for-sending-actual-attachments-in-email
…3-ui-for-sending-actual-attachments-in-email
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 12, 2026

@dev-rb dev-rb marked this pull request as ready for review January 12, 2026 22:36
@dev-rb dev-rb requested a review from a team as a code owner January 12, 2026 22:36
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 12, 2026

Code Review

Found 2 critical logic errors in createEmailFormState.ts that will cause data loss.


Issue 1: Logic Error in removeByFile (line 258)

Location: createEmailFormState.ts:256-259

Problem: The filter logic will incorrectly remove ALL remote attachments when trying to remove a single local attachment.

Current code:

removeByFile: (file: File) => {
  setAttachments((p) =>
    p.filter((a) => a.type === 'local' && a.file \!== file)
  );
},

This filter only keeps attachments that satisfy BOTH:

  • a.type === 'local' (is a local attachment) AND
  • a.file \!== file (doesn't match the file being removed)

Issue: All remote attachments fail the first condition, so they are all removed.

Suggested fix:

removeByFile: (file: File) => {
  setAttachments((p) =>
    p.filter((a) => a.type \!== 'local' || a.file \!== file)
  );
},

This correctly keeps:

  • All remote attachments (since a.type \!== 'local' is true)
  • All local attachments except the one being removed

Issue 2: Logic Error in removeByID (line 264)

Location: createEmailFormState.ts:261-266

Problem: The filter logic will incorrectly remove ALL local attachments when trying to remove a single remote attachment.

Current code:

removeByID: (attachmentID: string) => {
  setAttachments((p) =>
    p.filter(
      (a) => a.type === 'remote' && a.attachmentID \!== attachmentID
    )
  );
},

This filter only keeps attachments that satisfy BOTH:

  • a.type === 'remote' (is a remote attachment) AND
  • a.attachmentID \!== attachmentID (doesn't match the ID being removed)

Issue: All local attachments fail the first condition, so they are all removed.

Suggested fix:

removeByID: (attachmentID: string) => {
  setAttachments((p) =>
    p.filter(
      (a) => a.type \!== 'remote' || a.attachmentID \!== attachmentID
    )
  );
},

This correctly keeps:

  • All local attachments (since a.type \!== 'remote' is true)
  • All remote attachments except the one being removed

Both of these bugs would cause unintended data loss when users try to remove attachments from email drafts.

Copy link
Copy Markdown
Contributor

@synoet synoet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good

@dev-rb dev-rb merged commit 62e3d93 into main Jan 14, 2026
21 checks passed
@dev-rb dev-rb deleted the rahul/comms-103-ui-for-sending-actual-attachments-in-email branch January 14, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants