fix drafts and file uploads

assets/controllers/compose_controller.js

@@ -18,6 +18,9 @@ export default class extends Controller {
         minChars:     { type: Number, default: 5 },
         // The conversation this window belongs to, for the draft row.
         thread:       Number,
+        // Mirrors ComposeController::MAX_ATTACHMENT_BYTES so an oversized file
+        // is refused here rather than after the whole upload.
+        maxAttachmentBytes: { type: Number, default: 25 * 1024 * 1024 },
     }
 
     #autosaveTimer = null
@@ -482,6 +485,15 @@ export default class extends Controller {
             params.set('thread', String(this.threadValue));
         }
 
+        // Which list is behind the window. The Drafts view shows a row per
+        // draft, so a conversation that loses its last one has to lose its row
+        // there — and only there.
+        const scope = document.getElementById('message-list')?.dataset.syncScope;
+
+        if (undefined !== scope) {
+            params.set('scope', scope);
+        }
+
         const response = await fetch(`/compose/discard/${id}?${params}`, {
             method: 'POST',
             headers: { 'X-Requested-With': 'XMLHttpRequest' },
@@ -738,6 +750,15 @@ export default class extends Controller {
         // again after a removal.
         input.value = '';
 
+        const tooBig = files.find((file) => file.size > this.maxAttachmentBytesValue);
+
+        if (undefined !== tooBig) {
+            const limit = Math.round(this.maxAttachmentBytesValue / (1024 * 1024));
+            this._setStatus(`${tooBig.name} is over ${limit} MB`, 'text-danger');
+
+            return;
+        }
+
         await this.saveDraft(null, { force: true, allowEmpty: true });
 
         const id = this._messageId();
@@ -792,10 +813,7 @@ export default class extends Controller {
         }
 
         if (false === response.ok) {
-            this._setStatus(
-                413 === response.status ? 'Attachment too large' : failureMessage,
-                'text-danger',
-            );
+            this._setStatus(await this._errorText(response, failureMessage), 'text-danger');
 
             return;
         }
@@ -807,6 +825,21 @@ export default class extends Controller {
         this._setStatus('Draft saved', 'text-success');
     }
 
+    /**
+     * The server states why an upload was refused in plain text. Anything else
+     * — an HTML error page, a proxy cutting the request off — falls back to the
+     * generic message rather than dumping markup into the status line.
+     */
+    async _errorText(response, fallback) {
+        if (false === (response.headers.get('Content-Type') ?? '').startsWith('text/plain')) {
+            return fallback;
+        }
+
+        const detail = (await response.text()).trim();
+
+        return '' === detail ? fallback : detail;
+    }
+
     // ── Cc / Bcc ──────────────────────────────────────────────────────
 
     showCc() {

frankenphp/conf.d/10-app.ini

@@ -17,3 +17,12 @@ realpath_cache_size = 4096K
 realpath_cache_ttl = 600
 
 memory_limit = 2G
+
+; Compose attachments. PHP defaults (2M/8M) rejected anything past a small
+; image before the request ever reached Symfony. Keep post_max_size above
+; upload_max_filesize * max_file_uploads is not worth it — one oversized file
+; is refused per-file with a message, whereas breaching post_max_size wipes
+; $_POST entirely — so give the whole POST comfortable headroom instead.
+; ComposeController::MAX_ATTACHMENT_BYTES is the limit users actually meet.
+upload_max_filesize = 25M
+post_max_size = 60M

src/Controller/ComposeController.php

@@ -9,6 +9,7 @@ use App\Domain\Helper\AttachmentStorageHelper;
 use App\Entity\Account;
 use App\Entity\Message;
 use App\Entity\MessagePart;
+use App\Entity\MessageThread;
 use App\Form\ComposeType;
 use App\Infrastructure\Messaging\Message\SendMessageMessage;
 use App\Repository\AccountRepository;
@@ -46,8 +47,12 @@ class ComposeController extends AbstractController
     private const string INLINE_FRAME = 'compose_inline';
     private const string INLINE_FORM  = 'compose_inline';
 
-    /** Per-file ceiling for compose attachments. */
-    private const int MAX_ATTACHMENT_BYTES = 25 * 1024 * 1024;
+    /**
+     * Per-file ceiling for compose attachments. Public because the compose
+     * window reads it too, to refuse an oversized file before it is uploaded.
+     * Must stay under upload_max_filesize (frankenphp/conf.d/10-app.ini).
+     */
+    public const int MAX_ATTACHMENT_BYTES = 25 * 1024 * 1024;
 
     /** Matches the DelayStamp on the send job — the cancel window. */
     private const int SEND_DELAY_MS = 10_000;
@@ -316,17 +321,30 @@ class ComposeController extends AbstractController
         $this->assertDraft($message);
 
         $account = $message->getAccount();
-        $files   = $request->files->all('files');
+        $files   = array_filter(
+            $request->files->all('files'),
+            static fn (mixed $file): bool => $file instanceof UploadedFile,
+        );
+
+        // Nothing arrived at all. Almost always post_max_size: PHP discards the
+        // whole body, so $_FILES is empty and there is no per-file error to
+        // read — silence here is what made an oversized upload look like a
+        // no-op instead of a refusal.
+        if (0 === count($files)) {
+            return $this->uploadError('Upload too large');
+        }
+
+        // Everything is checked before anything is stored, so a rejected file
+        // cannot leave the ones before it attached.
+        foreach ($files as $file) {
+            $error = $this->attachmentError($file);
+
+            if (null !== $error) {
+                return $this->uploadError($error);
+            }
+        }
 
         foreach ($files as $file) {
-            if (false === $file instanceof UploadedFile || false === $file->isValid()) {
-                continue;
-            }
-
-            if ($file->getSize() > self::MAX_ATTACHMENT_BYTES) {
-                return new Response('Attachment too large.', Response::HTTP_REQUEST_ENTITY_TOO_LARGE);
-            }
-
             // Bucketed like synced attachments: account / mailbox (0 where the
             // account has none) / message. Drafts have no UID, so the message
             // id keeps one draft's files out of another's directory.
@@ -359,6 +377,40 @@ class ComposeController extends AbstractController
         return $this->render('compose/_attachments.html.twig', ['message' => $message]);
     }
 
+    /**
+     * Why this upload cannot be attached, or null when it can. Short enough to
+     * show in the window's status line.
+     */
+    private function attachmentError(UploadedFile $file): ?string
+    {
+        if (UPLOAD_ERR_INI_SIZE === $file->getError() || UPLOAD_ERR_FORM_SIZE === $file->getError()) {
+            return 'File too large';
+        }
+
+        if (false === $file->isValid()) {
+            return 'Upload failed';
+        }
+
+        if ($file->getSize() > self::MAX_ATTACHMENT_BYTES) {
+            return sprintf('File too large (max %d MB)', intdiv(self::MAX_ATTACHMENT_BYTES, 1024 * 1024));
+        }
+
+        return null;
+    }
+
+    /**
+     * Plain text so the window can show the reason as-is; the HTML answer of a
+     * successful upload is the attachment strip.
+     */
+    private function uploadError(string $message): Response
+    {
+        return new Response(
+            $message,
+            Response::HTTP_REQUEST_ENTITY_TOO_LARGE,
+            ['Content-Type' => 'text/plain; charset=utf-8'],
+        );
+    }
+
     #[Route('/attachment/{id}/remove', name: 'attachment_remove', methods: ['POST'])]
     public function removeAttachment(MessagePart $part): Response
     {
@@ -401,8 +453,11 @@ class ComposeController extends AbstractController
         // drifts, and the thread cascades removes to every message in it.
         // An emptied thread is left in place: harmless, and the sync layer
         // reuses it if the conversation comes back.
+        $remaining = 0;
+
         if (null !== $thread) {
-            $thread->setMessageCount(max(0, $thread->getMessages()->count() - 1));
+            $remaining = max(0, $thread->getMessages()->count() - 1);
+            $thread->setMessageCount($remaining);
         }
 
         $this->em->flush();
@@ -416,11 +471,51 @@ class ComposeController extends AbstractController
             'messageId' => $messageId,
             'frame'     => $ctx['frame'],
             'inline'    => $ctx['inline'],
+            'threadId'  => $this->rowToDrop($thread, $messageId, $remaining, $request),
         ]);
     }
 
     // ── Private helpers ───────────────────────────────────────────────────────
 
+    /**
+     * The thread whose list row the discard should take with it, or null to
+     * leave every row standing.
+     *
+     * List rows stand for threads, so an emptied thread loses its row wherever
+     * it is shown. The Drafts list is the other case: its rows are there
+     * because of a draft, so a conversation that just lost its last one drops
+     * out of that view even though the thread lives on. Which view is asking
+     * comes from the window (`scope`), because the same thread must keep its
+     * row in the Inbox.
+     */
+    private function rowToDrop(?MessageThread $thread, ?int $discardedId, int $remaining, Request $request): ?int
+    {
+        if (null === $thread) {
+            return null;
+        }
+
+        if (0 === $remaining) {
+            return $thread->getId();
+        }
+
+        if ('drafts' !== $request->query->get('scope')) {
+            return null;
+        }
+
+        foreach ($thread->getMessages() as $message) {
+            // The discarded message can still sit in the loaded collection.
+            if ($message->getId() === $discardedId) {
+                continue;
+            }
+
+            if (true === $message->isDraft()) {
+                return null;
+            }
+        }
+
+        return $thread->getId();
+    }
+
     /**
      * Where this compose window lives. The dock is the default; the thread
      * view passes ?frame=compose_inline&thread={id} on every URL it hands to

templates/_partials/_thread_row.html.twig

@@ -10,7 +10,12 @@ only when the row IS the draft — i.e. a thread holding a single draft
 message, or a bare Message row flagged as a draft. A real conversation
 carrying an unsent reply still opens the thread; that draft is edited from
 inside the reading pane.
+
+`draft_scope` (the Drafts list) overrides that: every row there is listed
+because of its draft, so it opens that draft even when the thread also holds
+the conversation it answers.
 #}
+{% set draft_scope = draft_scope|default(false) %}
 {% set item = thread ?? message %}
 {% set isThread = item.messages is defined %}
 
@@ -31,8 +36,12 @@ inside the reading pane.
        A thread can legitimately end up with no messages (every message
        expunged provider-side), and this row is rendered for the whole inbox,
        so one such thread would 500 the entire list. #}
-    {% set draft = latest and latest.isDraft and item.messageCount == 1 %}
-    {% set draftId = draft ? latest.id : null %}
+    {# Not `latest`: the collection is not ordered by date, so the draft is
+       looked up by flag rather than assumed to be the last message. #}
+    {% set draftMessage = item.messages|filter(m => m.isDraft)|last %}
+    {% set draft = draftMessage
+        and (draft_scope or item.messageCount == 1) %}
+    {% set draftId = draft ? draftMessage.id : null %}
 
     {% set hasAttachment = item.attachmentCount > 0 %}
 

templates/compose/_discard.stream.html.twig

@@ -1,14 +1,23 @@
 {#
 compose/_discard.stream.html.twig
 ─────────────────────────────────────────────────────────────────────────────
-The draft is gone: drop its row from the conversation and empty the window's
+The draft is gone: drop every row that stood for it and empty the window's
 frame. For an in-place draft the frame lives inside the row, so the remove
 already takes it with it.
 
-Variables: messageId, frame, inline
+Both removes address ids that only exist in the view the draft was opened
+from — the conversation row when reading a thread, the list row in Drafts —
+and a turbo-stream remove aimed at an absent id is a no-op, so each view gets
+exactly the one that applies to it.
+
+Variables: messageId, frame, inline, threadId (null unless the thread is now
+empty)
 #}
-{% if inline %}
-    <turbo-stream action="remove" target="thread_message_{{ messageId }}">
+<turbo-stream action="remove" target="thread_message_{{ messageId }}">
+</turbo-stream>
+
+{% if threadId is not null %}
+    <turbo-stream action="remove" target="thread_{{ threadId }}">
     </turbo-stream>
 {% endif %}
 

templates/compose/_window.html.twig

@@ -24,6 +24,7 @@ Variables: form, message, inline (bool), frame (string), thread (int|null)
     <div data-controller="compose compose-toolbar"
          data-compose-inline-value="{{ inline ? 'true' : 'false' }}"
          data-compose-min-chars-value="{{ min_chars|default(5) }}"
+         data-compose-max-attachment-bytes-value="{{ constant('App\\Controller\\ComposeController::MAX_ATTACHMENT_BYTES') }}"
          {% if thread is not null %}data-compose-thread-value="{{ thread }}"{% endif %}
         {% if null == message.id %}
             data-compose-draft-url-value="{{ path('app_compose_form_new', ctx) }}"

templates/mail/drafts.html.twig

@@ -20,7 +20,7 @@
         </li>
     {% else %}
         {% for thread in threads %}
-            {% include '_partials/_thread_row.html.twig' with { thread: thread } %}
+            {% include '_partials/_thread_row.html.twig' with { thread: thread, draft_scope: true } %}
         {% endfor %}
     {% endif %}
 {% endblock %}

tests/e2e/compose.spec.ts

@@ -123,6 +123,24 @@ test.describe("compose window", () => {
         await expect(chip).toBeHidden();
     });
 
+    // PHP ships upload_max_filesize=2M / post_max_size=8M, which silently ate
+    // anything bigger before frankenphp/conf.d/10-app.ini raised them.
+    test("attaches a file larger than PHP's stock upload limit", async ({ page }) => {
+        await page.goto("/mail/inbox");
+        await page.getByRole("link", { name: "Compose" }).click();
+
+        const dockEl = page.locator(dock);
+
+        await dockEl.locator('input[type="file"]').setInputFiles({
+            name: "e2e-big.bin",
+            mimeType: "application/octet-stream",
+            buffer: Buffer.alloc(3 * 1024 * 1024, 7),
+        });
+
+        await expect(dockEl.getByRole("link", { name: "e2e-big.bin" })).toBeVisible();
+        await expect(dockEl.getByText("3.0 MB")).toBeVisible();
+    });
+
     // `allow_options_create => true` never reached Tom Select — this version of
     // the bundle renders it as a valueless boolean attribute that Stimulus reads
     // back as false — so ContactAutocompleteField now passes `create` through

tests/e2e/tmp-drafts.spec.ts

@@ -0,0 +1,36 @@
+import { test, expect } from "@playwright/test";
+import { mailRow } from "./support/config";
+
+test("make a reply draft inside a conversation", async ({ page }) => {
+    await page.goto("/mail/inbox");
+    await mailRow(page, "E2E Read Me").click();
+
+    await page.getByRole("link", { name: "Reply", exact: true }).first().click();
+    await page
+        .locator('#compose_inline [data-compose-toolbar-target="editor"]')
+        .fill("Reply draft body");
+
+    await page.waitForResponse((r) =>
+        r.url().includes("/compose/draft") && r.request().method() === "POST"
+    );
+});
+
+test("conversation row in Drafts opens the draft and discards live", async ({ page }) => {
+    await page.goto("/mail/drafts");
+
+    const row = page.locator("#message-list li").filter({ hasText: "E2E Read Me" });
+    await expect(row).toBeVisible();
+
+    await row.click();
+
+    // draft_scope: the row opens the draft in the dock, not the thread.
+    const dockEl = page.locator("#compose_dock");
+    await expect(dockEl.locator('[data-compose-toolbar-target="editor"]')).toContainText(
+        "Reply draft body",
+    );
+
+    await dockEl.getByRole("button", { name: "Delete draft" }).click();
+
+    // The thread lives on, but it has no draft left, so it leaves this view.
+    await expect(row).toHaveCount(0);
+});