Add support for dragging threads into composer as .eml attachments - #2800
Add support for dragging threads into composer as .eml attachments#2800bengotow wants to merge 3 commits into
Conversation
Dragging a thread out of the thread list and dropping it on an open composer now attaches it as a .eml file, the way Outlook and Gmail do. Thread rows already publish their ids on `mailspring-threads-data` for the folder-drop feature, so the composer's DropZone just had to learn to accept that type. The .eml itself is materialized on drop rather than on dragstart: fetching the raw RFC2822 source is a round trip to the sync engine, and `dragstart` has to populate dataTransfer synchronously. While the fetch is in flight the attachments area shows a placeholder. Dropping a thread onto a reply being composed inside that same thread is ignored — attaching a conversation to itself isn't useful. The staging logic (pick the thread's representative message, fetch it, write it to a temp file) was already duplicated between "Forward as Attachment" in the message list and the thread list context menu, so it moves into EmlUtils alongside defaultEmlFilename and all four call sites share it. Attachment names are unchanged for the existing features; dragged-in messages are named after their subject. Drafts are now excluded when picking a thread's representative message. They only exist locally, so the sync engine has no raw source to return for them — previously an unsent draft could be picked as the newest message in its thread and the export would silently produce nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQrDtxTjqZV5ms7W7Jp91m
|
Warning Indent Zero is shutting down on August 7th. Please migrate over to Indent 2.0 to continue getting PR reviews.
|
|
![indent[bot]](https://avatars.githubusercontent.com/in/4161513?s=60&v=4){kind=small}
![indent-staging[bot]](https://avatars.githubusercontent.com/u/174392868?s=60&v=4){kind=small}
Three fixes from PR review on the drag-to-attach change.
Staged .eml files are now cleaned up. Staging moved from the old
`mailspring-fwd-${message.id}` path — bounded per message, so a repeat
forward overwrote it — to a randomly named directory, which grows without
limit. stageMessagesAsEml now removes the directories of messages whose
file never arrived, and discardStagedEml lets callers drop the rest once
they're done; the attachment store copies the file into its own directory
before addAttachment's onCreated fires, so that's the point where the
staged copy becomes garbage. It refuses any directory not named
`mailspring-eml-*`, so a stray path can't take a real directory with it.
A thread with nothing exportable in it is no longer reported as a failed
download. stageThreadsAsEml separates the two: a conversation holding
only unsent drafts is never fetched at all, so "Please try again" was
both wrong and unactionable. They now get distinct messages, and the
failure count no longer includes threads that were never fetched.
Dropping a thread onto a reply composed within that same thread is now
allowed. The payload isn't readable during dragEnter, so the drop cover
had already appeared by the time the id was filtered out — the drop
looked accepted and then did nothing. Outlook and Gmail both allow it,
and the guard was speculative.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TQrDtxTjqZV5ms7W7Jp91m
|
All three review findings addressed in aabcf6c. Temp directory leak — valid, and a regression this PR introduced. The old path was Misleading download-failed dialog — valid. Same-thread drop swallowed silently — valid, but resolved by removing the guard rather than by adding a notice. The payload isn't readable during Spec suite: 1486 passing, 0 failing (up from 1480 — 6 new specs covering staging cleanup, the discard guard, and the unavailable-vs-failed split). Generated by Claude Code |
| } catch (err) { | ||
| return; | ||
| } | ||
| if (!threadIds.length) { |
There was a problem hiding this comment.
The self-drop guard that was here in the first commit (threadIds = threadIds.filter((id) => id !== this.props.draft.threadId);) was removed in aabcf6c and not replaced. For a reply draft, draft.threadId is set, so dragging that same thread onto the composer now stages its newest non-draft message and attaches the conversation to itself — the "almost always an accident" case the earlier commit deliberately suppressed, and which the PR description still lists ("Filters out drops of threads onto themselves"). If this removal was intentional, update the description; otherwise restore the filter. New-compose drafts have a null threadId, so they're unaffected either way.
| <RetinaImg name="inline-loading-spinner.gif" mode={RetinaImg.Mode.ContentPreserve} /> | ||
| <span> | ||
| {attachingThreadCount === 1 | ||
| ? localized('Attaching message…') |
There was a problem hiding this comment.
Can we collapse this into just Attaching... and remove the attachingThreadCount and rename the prop to attaching: boolean, I think that'd be cleaner and make this more portable to future use
There was a problem hiding this comment.
I think we do have to keep the state at the composer-view layer for tracking when we're done, just remove it from this component
| ))} | ||
| {attachingThreadCount > 0 && ( | ||
| <div className="attaching-messages"> | ||
| <RetinaImg name="inline-loading-spinner.gif" mode={RetinaImg.Mode.ContentPreserve} /> |
There was a problem hiding this comment.
This image looks like shit, can you use the Spinner component? In a follow-up we'll clean up other remaining uses of this. The problem is it doesn't look good in dark mode and can't be inverted.
| // Accept drops of threads from the thread list, attaching each one as a | ||
| // .eml file. dataTransfer is only valid for the duration of this handler, | ||
| // so read the payload now and hand the ids off to an async worker. |
There was a problem hiding this comment.
please remove this comment it's self explanatory
| // type. Note that we can only look at `types` here — the payload itself is | ||
| // not readable until the drop actually happens. | ||
| _hasThreadsForDrop = (event: React.DragEvent<HTMLDivElement>) => { | ||
| return event.dataTransfer.types.includes('mailspring-threads-data'); |
There was a problem hiding this comment.
can you make mailspring-threads-data a constant and import it?
| : localized( | ||
| '%1$@ of the conversations have no message that can be attached.', | ||
| unavailableThreadIds.length | ||
| ) |
There was a problem hiding this comment.
Can we make this say "one or more" so it's a single localized string, just to simplify the amount of localization required
| ? localized('Could not download the original message. Please try again.') | ||
| : localized( | ||
| 'Could not download %1$@ of the original messages. Please try again.', | ||
| failedCount |
There was a problem hiding this comment.
same with this one, let's make it a single error dialog by rephrasing it to say one or more
| const message = messages[0]; | ||
| const pathModule = require('path'); | ||
| const fs = require('fs'); | ||
| const tempDir = pathModule.join( | ||
| require('@electron/remote').app.getPath('temp'), | ||
| `mailspring-fwd-${message.id}` | ||
| ); | ||
| fs.mkdirSync(tempDir, { recursive: true }); | ||
| const tempPath = pathModule.join(tempDir, 'Forwarded Message.eml'); | ||
|
|
||
| const task = new GetMessageRFC2822Task({ | ||
| messageId: message.id, | ||
| accountId: message.accountId, | ||
| filepath: tempPath, | ||
| const staged = await EmlUtils.stageMessagesAsEml([message], { |
There was a problem hiding this comment.
can we write a wrapper stageMessageAsEml (singular) and do the array wrapping and unwrapping in there in the EmlUtils? It feels like this is awkward and we repeatedly reference staged[0] - i think at the very least it should be destructured right here so it's all colocated in these three lines
There was a problem hiding this comment.
edit: i think we actually want stageThreadAsEml (signular)
| const yes: T[] = []; | ||
| const no: T[] = []; | ||
| items.forEach((item) => (predicate(item) ? yes : no).push(item)); | ||
| return [yes, no]; |
There was a problem hiding this comment.
I think we have either lodash or underscore, lets just use that
|
|
||
| // Placeholder shown while a thread dragged onto the composer is being fetched | ||
| // from the sync engine and written out as a .eml file. | ||
| .attaching-messages { |
There was a problem hiding this comment.
can you align this with the real attachment items better?
- Use the Spinner component for the attaching placeholder instead of inline-loading-spinner.gif, which can't be inverted for dark mode. - Collapse the placeholder to "Attaching…" and pass AttachmentsArea a plain `attaching` boolean. ComposerView keeps the count, since it still needs it to know when the last in-flight drop has landed. - Line the placeholder up with .nylas-attachment-item, matching its left indent and font size. - Add stageMessageAsEml and stageThreadAsEml, singular wrappers that return StagedEml | null, so callers staging one thing stop indexing into an array. thread-list-context-menu now goes straight from a thread id to a staged file and destructures it once. - Collapse the two drop failure dialogs into one, phrased "one or more" so no count has to be interpolated and both cases can be reported together rather than stacking two dialogs. - Move the 'mailspring-threads-data' and 'mailspring-accounts=' magic strings into DragDropTypes, shared by the thread list that writes them and the composer and folder sidebar that read them. - Use underscore's partition instead of a hand-rolled one. - Drop a comment that restated the code below it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQrDtxTjqZV5ms7W7Jp91m
|
Thanks @bengotow — all eleven addressed in b346a73, plus a PR description rewrite (it still described the self-drop filter that aabcf6c removed, which is what the bot kept flagging).
One thing worth confirming: the drop is only wired to the inline composer. A drag into a popout composer window won't register — Chromium doesn't carry custom Local suite: 1489 passing, 0 failing. Generated by Claude Code |
|
@claude this is working great, but with one exception. You can drag and drop anywhere into the composer EXCEPT over the current draft text area (where the body text of the draft appears). If you drop into the composer header, it works, if you drop below the "quoted text expand/collapse button" it works, it's just the area right in the center that cannot be dropped on. Unfortunately this is the primary area people will try to drop, maybe it's triggering some inline file attachment code? |
2 participants
Summary
Dragging a thread out of the thread list and dropping it on an open composer now attaches it as a
.emlfile, the way Outlook and Gmail do.Most of the machinery already existed. Thread rows publish their ids on
mailspring-threads-datafor the folder-drop feature, and the composer is already wrapped in aDropZonewith a "Drop to Attach" overlay — the composer just had to learn to accept that type.GetMessageRFC2822Task+Actions.addAttachmentare what "Forward as Attachment" already used to turn a message into a.emland hang it off a draft.The
.emlis materialized on drop, not on dragstart:dragstartmust populatedataTransfersynchronously, but fetching raw RFC2822 source is a round trip to the sync engine. Since the drop target is in-app, only thread ids are needed at dragstart.Key Changes
Shared staging helpers (
app/src/services/eml-utils.ts) — the "pick the thread's representative message → fetch raw source → write a temp file" sequence was copy-pasted between the message list and the thread list context menu. It now lives here and all four call sites share it:newestExportableMessagesForThreadIds()— newest non-draft message per threadstageMessagesAsEml()/stageMessageAsEml()— fetch and write to isolated temp.emlfilesstageThreadsAsEml()/stageThreadAsEml()— thread ids straight through to staged filesdiscardStagedEml()— drop a staged file once the caller is done with itComposer drop handling (
composer-view.tsx) — acceptsDragDropTypes.ThreadsDragType, reads the ids synchronously in_onDrop, then stages and attaches asynchronously.Attachments area (
attachments-area.tsx) — anattachingboolean renders aSpinner+ "Attaching…" placeholder while files are being materialized.Drag-type constants (
app/src/drag-drop-types.ts) —mailspring-threads-dataandmailspring-accounts=are no longer magic strings; the thread list that writes them and the composer and folder sidebar that read them share the constants.Test coverage (
app/spec/services/eml-utils-spec.ts) — the staging helpers, temp-directory cleanup, thediscardStagedEmlprefix guard, and the unavailable-vs-failed split. Suite is at 1489 passing.Behavior changes beyond the new feature
Forwarded Message.eml); dragged-in messages are named after their subject.dragEnter, so the drop cover had already appeared by the time the id was dropped — the drop looked accepted and then did nothing. Outlook and Gmail both allow it.Implementation notes
GetMessageRFC2822Taskcan reachcompletewithout having written anything, so results are filtered by file existence; those staging directories are removed immediately, and the rest are freed fromaddAttachment'sonCreated, which fires after the attachment store has copied the file into its own directory.Known limitation
This works for the inline composer (replying/forwarding in the main window). It does not work for popout composer windows — Chromium doesn't carry custom
dataTransfertypes across ElectronBrowserWindowboundaries — and it doesn't enable drag-to-Desktop. Both needwebContents.startDrag, which requires the file to exist before the drag begins, i.e. speculative pre-fetching on mousedown. That's a different mechanism and is left out rather than half-built.https://claude.ai/code/session_01TQrDtxTjqZV5ms7W7Jp91m