Skip to content

fix(email): attachment download bytes, forward loses subject/attachments/body - #355

Open
DiogoDuart3 wants to merge 1 commit into
oblien:mainfrom
DiogoDuart3:pr1-attachment-forward-fix
Open

fix(email): attachment download bytes, forward loses subject/attachments/body#355
DiogoDuart3 wants to merge 1 commit into
oblien:mainfrom
DiogoDuart3:pr1-attachment-forward-fix

Conversation

@DiogoDuart3

Copy link
Copy Markdown
Contributor

Bugs

Attachment download always fails ("Attachment data not found"): getMessageAttachments returns metadata with body: '' for every attachment. simpleParser already decodes each attachment's bytes (a.content) while building the message body, so base64-encoding it into the response costs no extra IMAP round trip - it's already sitting in memory.

Forwarding a message drops the subject, attachments, and body:

  • The composer never seeds a subject for forward mode (RHF defaultValues.subject stays '').
  • The attachment "chip" visible while composing a forward is the read-pane's display of the original message - it was never wired into the composer's own file list, so the outgoing payload always has attachments: [].
  • The composed HTML ends at the "---------- Forwarded message ----------" header block. The original body only travels in a separate originalMessage field, which the server's mail.send input schema validated but never used - producing a bodyless forward.

Fix

  • Populate body with a.content.toString('base64') in the attachment mapping (getThread).
  • Seed Fwd: <subject> client-side when entering forward mode with no existing draft.
  • Server-side, when isForward + originalMessageId are present: fetch the original message's attachments straight from IMAP (via getThread, now returning real bytes) instead of trusting the client payload, and append originalMessage (already HTML-preferring) into the outgoing body after the forwarded-header block.

Verified end-to-end: attachment download yields a byte-identical file; a fresh forward arrives with the seeded subject, the original attachment, and the original body appended.

Note on CI

Recent merged PRs against main (#322, #327) show Typecheck: SUCCESS / Test: FAILURE on their PR-context runs, while direct pushes to main show Test: SUCCESS - this looks like a pre-existing PR-vs-push CI environment difference, not something introduced here. Flagging in case it reproduces on this PR too.

…nts/body

getMessageAttachments always returned metadata with body:'' - simpleParser
already decodes each attachment's bytes (a.content) to build the message
body, so base64-encoding it into the response costs no extra IMAP round
trip. Fixes "Attachment data not found" on every download.

Forward composer never seeded a subject (RHF defaultValues.subject stayed
'') and always sent attachments:[] - the visible chip while forwarding is
the read-pane's display of the ORIGINAL message, never wired into the
composer's own file list. Seed "Fwd: <subject>" client-side; server-side,
when isForward + originalMessageId are present, fetch the original
message's attachments straight from IMAP (via getThread, now returning
real bytes) instead of trusting the client payload, and append
originalMessage (already HTML-preferring) into the outgoing body after the
"Forwarded message" header block - previously validated by the input
schema and silently dropped, producing bodyless forwards.

Verified end-to-end: attachment download yields a byte-identical file; a
fresh forward arrives with the seeded subject, the original attachment,
and the original body appended.
@Hydralerne

Copy link
Copy Markdown
Member

Diagnoses are all correct, and the forward server-side handling is solid — fetching the original's attachments from IMAP instead of trusting the client is the right call, and the {name,type,base64} shape lines up cleanly with the send schema and driverSend's Buffer.from(a.base64,'base64').

One blocker on Fix 1: populating body in getThread ships every attachment's full bytes (base64, +33%) on the read path, not just download. getThread feeds get (reading pane) and drafts.get, not only getMessageAttachments — so opening a message with a 25 MB attachment now transfers ~33 MB just to render a chip. It also contradicts the invariant documented at imap-driver.ts:288 ("body is empty on list/thread reads — the download path returns it separately"), which the PR doesn't update.

Suggested fix: thread an includeAttachmentBytes flag through getThread (default false), and set it true only in getMessageAttachments and the forward branch of send. Read path stays lean, invariant preserved.

Nits: forwarding a Fwd: … subject double-prefixes; and forwardAttachments ?? input.attachments will clobber user-added attachments once the composer's forward file list is wired (harmless today).

Functionally correct — just needs the byte-gating before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants