Move a thread's unread count when JMAP marks a message read or unread (#267)

EmailPatchApplier wrote seenAt for $seen and left MessageThread::$unreadCount alone, so the web list (which draws bold from that counter) disagreed with the phone app. $seen now goes through ThreadStatusUpdater::markRead(), the web UI's own path, and markRead() counts the whole thread rather than the messages passed in.

Closes #266

Co-authored-by: Philippe Delodder <7259755+phdelodder@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -55,6 +55,16 @@ skips its "View in browser" lines; a person's mail starts at its first line, gre
 The Android and iOS apps get the same preview. No schema change.
 [#257](https://github.com/karatektus/pl_mail/issues/257).
 
+**A message marked read or unread in the phone app is shown that way on the web too.** Marking a
+message read or unread over JMAP changed the message but not its conversation's unread count, which
+is what the web list draws in bold from, so a message marked unread on the phone stayed read in the
+browser and one marked read there stayed bold. It now goes the same way as the web's own read
+button, and so does an automatic read receipt where the mailbox sends one. Marking a single message
+of a longer conversation read or unread on the web also set the conversation's count from that one
+message alone; it is now counted from all of them. A conversation whose count is already wrong is
+put right the next time one of its messages is marked. No schema change.
+[#266](https://github.com/karatektus/pl_mail/issues/266), by @phdelodder.
+
 ## v0.3.7 — 2026-10-20
 
 <!-- highlight screenshot=sso-only -->

src/Jmap/Mail/EmailPatchApplier.php

@@ -13,6 +13,7 @@ use App\Repository\Label\LabelBindingRepository;
 use App\Repository\Label\LabelRepository;
 use App\Service\Label\LabelChangePropagator;
 use App\Service\Label\ThreadLabelSynchronizer;
+use App\Service\Mail\ThreadStatusUpdater;
 
 /**
  * Applies a JMAP patch to an Email's keywords and mailboxIds.
@@ -25,6 +26,11 @@ use App\Service\Label\ThreadLabelSynchronizer;
  * one made in the browser. Its ordering contract is observed: mutate the
  * entities, call the propagator, and let the caller flush last — detachLabel
  * in particular reads message->mailbox before it is re-pointed.
+ *
+ * Read state is the exception: $seen goes through ThreadStatusUpdater, which
+ * flushes on its own, so that a phone marking a message read or unread keeps
+ * the thread's unread counter, the read receipt and the propagation exactly
+ * as the web UI does.
  */
 final class EmailPatchApplier
 {
@@ -34,6 +40,7 @@ final class EmailPatchApplier
         private readonly LabelChangePropagator $propagator,
         private readonly ThreadLabelSynchronizer $threadLabelSynchronizer,
         private readonly JmapDraftWriter $draftWriter,
+        private readonly ThreadStatusUpdater $status,
     ) {
     }
 
@@ -152,8 +159,13 @@ final class EmailPatchApplier
         $isFlagged = null !== $message->starredAt;
 
         if ($wantSeen !== $isSeen) {
-            $message->seenAt = true === $wantSeen ? new \DateTimeImmutable() : null;
-            $this->propagator->markRead([$message], $wantSeen);
+            // Through the same service as the web UI's read button, not by
+            // writing seenAt here. Writing it here left the thread's
+            // unreadCount alone, and the web list draws bold from that counter:
+            // a message marked unread on the phone stayed read in the browser,
+            // and one marked read there stayed bold. The service also tells
+            // the provider, so the propagator is not called a second time.
+            $this->status->markRead([$message], $wantSeen);
         }
 
         if ($wantFlagged !== $isFlagged) {

src/Service/Mail/ThreadStatusUpdater.php

@@ -542,19 +542,19 @@ final readonly class ThreadStatusUpdater
         // left the others untouched — so four conversations were marked read in
         // the database and three rows went on rendering as unread, because
         // `data-unread` reads the thread's counter rather than its messages.
-        $counts = [];
+        //
+        // Counted over ALL of the thread's messages, not only the ones passed
+        // in. Counting the set was the same mistake one level down: a single
+        // message marked unread from the thread view, or over JMAP by a phone,
+        // set its conversation's counter to 1 or 0 whatever the other messages
+        // in it said.
+        $threads = [];
 
         foreach ($messages as $message) {
             $thread = $message->thread;
 
-            if (null === $thread) {
-                continue;
-            }
-
-            $counts[spl_object_id($thread)] ??= ['thread' => $thread, 'unread' => 0];
-
-            if (null === $message->seenAt) {
-                ++$counts[spl_object_id($thread)]['unread'];
+            if (null !== $thread) {
+                $threads[spl_object_id($thread)] = $thread;
             }
         }
 
@@ -563,7 +563,15 @@ final readonly class ThreadStatusUpdater
         // badges are drawn from the counter.
         $counterMoved = false;
 
-        foreach ($counts as ['thread' => $thread, 'unread' => $threadUnread]) {
+        foreach ($threads as $thread) {
+            $threadUnread = 0;
+
+            foreach ($thread->messages as $message) {
+                if (null === $message->seenAt) {
+                    ++$threadUnread;
+                }
+            }
+
             if ($thread->unreadCount !== $threadUnread) {
                 $thread->unreadCount = $threadUnread;
                 $counterMoved        = true;
@@ -594,16 +602,17 @@ final readonly class ThreadStatusUpdater
      * THIS METHOD IS THE ONE THAT MUST NOT BE MOVED. It hangs off markRead(),
      * which is the user-initiated path and nothing else: the thread view's
      * auto-mark on open, the row's mark-read button, the toolbar's bulk
-     * action. Its inbound twin — applyRemoteFlags(), thirty lines down — takes
-     * the provider's word for a flag and deliberately does not call this, so a
-     * sync that discovers a message was already \Seen on another device sends
-     * nothing. That distinction is the entire correctness question in read
-     * receipts: a receipt claims a person displayed the message, and a sync
-     * pass learning about a read that happened elsewhere, possibly weeks ago,
-     * cannot make that claim. Two other writers of seenAt exist and are
-     * likewise silent — the rule engine's markRead action, which is software
-     * filing mail rather than a person reading it, and ThreadSnoozeService,
-     * which only ever un-reads.
+     * action, and a JMAP client setting $seen, which is the app on a phone
+     * doing the same three things. Its inbound twin — applyRemoteFlags(),
+     * thirty lines down — takes the provider's word for a flag and
+     * deliberately does not call this, so a sync that discovers a message was
+     * already \Seen on another device sends nothing. That distinction is the
+     * entire correctness question in read receipts: a receipt claims a
+     * person displayed the message, and a sync pass learning about a read
+     * that happened elsewhere, possibly weeks ago, cannot make that claim.
+     * Two other writers of seenAt exist and are likewise silent — the rule
+     * engine's markRead action, which is software filing mail rather than a
+     * person reading it, and ThreadSnoozeService, which only ever un-reads.
      *
      * Only genuine transitions, so re-marking an already-read message read —
      * which the bulk toolbar does routinely over a mixed selection — cannot
@@ -689,8 +698,9 @@ final readonly class ThreadStatusUpdater
      *
      * Counters are recounted from the thread's own messages rather than
      * adjusted, because a flag pass can change several messages of one thread
-     * at once and in both directions; markRead()'s cheaper arithmetic works
-     * because the user's action covers the whole selection.
+     * at once and in both directions. markRead() counts the same way, for the
+     * same reason: a single message of a longer conversation is a selection
+     * too.
      *
      * @param list<RemoteFlagState> $states
      * @param DateTimeImmutable|null $readAt  when the provider was asked. The

tests/Jmap/Mail/EmailPatchApplierTest.php

@@ -18,6 +18,7 @@ use App\Service\Label\LabelResolver;
 use Doctrine\DBAL\Connection;
 use Doctrine\ORM\EntityManagerInterface;
 use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+use Symfony\Component\Messenger\Transport\InMemory\InMemoryTransport;
 
 /**
  * What a JMAP client may rewrite, and on what.
@@ -139,9 +140,8 @@ final class EmailPatchApplierTest extends KernelTestCase
     // ── Keywords still work ───────────────────────────────────────────────
 
     /**
-     * Read state is carried by seenAt, not by the SEEN flag — note this path
-     * differs from the web UI's, which sets both. Asserted on seenAt because
-     * that is what this code actually contracts to change.
+     * Read state is carried by seenAt and the SEEN flag together, the same as
+     * in the web UI, because both now go through ThreadStatusUpdater.
      */
     public function testSeenKeywordTogglesReadState(): void
     {
@@ -149,9 +149,72 @@ final class EmailPatchApplierTest extends KernelTestCase
 
         $this->applier->apply($this->account, $message, ['keywords/$seen' => true]);
         self::assertNotNull($message->seenAt);
+        self::assertTrue($message->hasFlag(MessageFlag::SEEN));
 
         $this->applier->apply($this->account, $message, ['keywords/$seen' => null]);
         self::assertNull($message->seenAt);
+        self::assertFalse($message->hasFlag(MessageFlag::SEEN));
+    }
+
+    /**
+     * The regression. The web list draws a conversation in bold from
+     * MessageThread::$unreadCount, and $seen used to write seenAt and leave
+     * the counter alone: read on the phone stayed bold in the browser, unread
+     * on the phone stayed plain.
+     */
+    public function testSeenKeywordMovesTheThreadUnreadCountBothWays(): void
+    {
+        $thread  = $this->thread(messages: 1);
+        $message = $thread->messages->first();
+
+        self::assertInstanceOf(Message::class, $message);
+
+        $this->applier->apply($this->account, $message, ['keywords/$seen' => true]);
+        self::assertSame(0, $thread->unreadCount, 'read over JMAP, still bold on the web');
+
+        $this->applier->apply($this->account, $message, ['keywords/$seen' => null]);
+        self::assertSame(1, $thread->unreadCount, 'unread over JMAP, still plain on the web');
+    }
+
+    /**
+     * Counted from the whole conversation, not from the one message the patch
+     * names: marking one of three unread messages read leaves two.
+     */
+    public function testSeenKeywordOnOneMessageCountsTheRestOfTheThread(): void
+    {
+        $thread   = $this->thread(messages: 3);
+        $messages = $thread->messages->toArray();
+
+        $this->applier->apply($this->account, $messages[0], ['keywords/$seen' => true]);
+        self::assertSame(2, $thread->unreadCount);
+
+        $this->applier->apply($this->account, $messages[1], ['keywords' => ['$seen' => true, '$flagged' => true]]);
+        self::assertSame(1, $thread->unreadCount);
+        self::assertNotNull($messages[1]->starredAt, '$flagged in the same patch still applies');
+
+        $this->applier->apply($this->account, $messages[0], ['keywords/$seen' => false]);
+        self::assertSame(2, $thread->unreadCount);
+    }
+
+    /**
+     * The provider is told once. The read now goes through ThreadStatusUpdater,
+     * which calls the propagator itself; calling it here as well would queue
+     * every read twice.
+     */
+    public function testSeenKeywordIsPropagatedOnce(): void
+    {
+        $thread  = $this->thread(messages: 1);
+        $message = $thread->messages->first();
+
+        self::assertInstanceOf(Message::class, $message);
+
+        $transport = self::getContainer()->get('messenger.transport.export');
+        self::assertInstanceOf(InMemoryTransport::class, $transport);
+        $transport->reset();
+
+        $this->applier->apply($this->account, $message, ['keywords/$seen' => true]);
+
+        self::assertCount(1, $transport->getSent());
     }
 
     public function testFlaggedKeywordTogglesStarred(): void
@@ -228,6 +291,42 @@ final class EmailPatchApplierTest extends KernelTestCase
 
     // ── Fixtures ──────────────────────────────────────────────────────────
 
+    /**
+     * A conversation of unread messages whose counter agrees with them. A
+     * mailbox and a UID on each, because LabelChangePropagator skips messages
+     * that have neither.
+     */
+    private function thread(int $messages): MessageThread
+    {
+        $thread = new MessageThread();
+        $thread->account = $this->account;
+        $thread->subject = 'Read fixture';
+        $thread->normalizedSubject = 'read fixture';
+        $thread->lastMessageAt = new \DateTimeImmutable('-1 hour');
+        $thread->threadingMethod = ThreadingMethod::References;
+        $thread->unreadCount = $messages;
+        $this->em->persist($thread);
+
+        for ($i = 0; $i < $messages; $i++) {
+            $message = new Message();
+            $message->account = $this->account;
+            $message->subject = sprintf('Read fixture %d', $i);
+            $message->fromAddress = 'sender@example.test';
+            $message->receivedAt = new \DateTimeImmutable('-1 hour');
+            $message->hasAttachments = false;
+            $message->messageId = sprintf('<read-%s-%d@example.test>', uniqid('', true), $i);
+            $message->mailbox = $this->mailbox;
+            $message->imapUid = 3000 + $i;
+
+            $thread->addMessage($message);
+            $this->em->persist($message);
+        }
+
+        $this->em->flush();
+
+        return $thread;
+    }
+
     private function message(bool $draft): Message
     {
         $thread = new MessageThread();

tests/Service/Mail/ThreadStatusUpdaterTest.php

@@ -305,6 +305,27 @@ final class ThreadStatusUpdaterTest extends KernelTestCase
         self::assertSame(3, $thread->unreadCount);
     }
 
+    /**
+     * One message of a longer conversation, as the thread view's per-message
+     * button and a JMAP client both send. The counter used to be counted from
+     * the messages passed in, so one message marked unread set a conversation
+     * with three unread messages to 1, and one marked read set it to 0.
+     */
+    public function testMarkingOneMessageCountsTheWholeThread(): void
+    {
+        $thread   = $this->inboxThread(3);
+        $messages = $thread->messages->toArray();
+
+        $this->updater->markRead([$messages[0]], true);
+        self::assertSame(2, $thread->unreadCount);
+
+        $this->updater->markRead([$messages[1], $messages[2]], true);
+        self::assertSame(0, $thread->unreadCount);
+
+        $this->updater->markRead([$messages[2]], false);
+        self::assertSame(1, $thread->unreadCount);
+    }
+
     /**
      * Opening a conversation marks it read every time, and most conversations
      * opened have been read before. Those calls used to be written all the