jody/pl_mail
1
0 Fork 0
mirror of https://github.com/karatektus/pl_mail.git synced 2026-10-25 10:04:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(mail): retire the "New" marker on display, and time it out after a day

Browse Source 
Three reports, one of which invalidated the feature.

THE BADGE SURVIVED EVERYTHING. Reproduced in Chromium before anything was
changed: hovering the sidebar's Inbox link fires GET /mail/inbox carrying
`X-Sec-Purpose: prefetch`, and the click that follows issues NO REQUEST AT ALL
— Turbo 8 serves the response it prefetched on hover. The request log for two
full visits shows two requests, both prefetches. listed_at stayed NULL and
data-new stayed "true" indefinitely, through navigation and through reload.

So the prefetch guard was not too broad, it was built on a false premise. It
declined to mark a speculative fetch, which is right, on the assumption that a
real request would follow if the user clicked — and for a prefetched link there
is no such request. The prefetched response IS the page the user looks at.

A server cannot tell a render from a display; only the document can. So the
rows report in: mail--new-marker sits on the list container, collects the ids
of rows carrying data-new-id once they are actually in the DOM and the tab is
actually visible, and POSTs them to /mail/threads/listed. Server-side marking
stays for the ordinary non-prefetched visit — it costs the client nothing and
keeps the no-JS path honest — and the two are idempotent against each other
because the UPDATE only touches rows still null. Ownership on that endpoint is
a WHERE clause, not an assumption, since the ids arrive off the wire.

Why the suite passed while this was live: every spec navigated with
page.goto(), which is a browser load with no Turbo, no hover and no prefetch —
so "badge gone" and "row marked" were the same observation. The new specs click
links, hover first, and read listed_at out of Postgres, which is the pair of
statements that can come apart. Removing the client half fails exactly those
three and nothing else.

"NEW" NOW TIMES OUT after 24 hours (MessageThread::NEW_WINDOW), because a
marker you can only clear by looking at every row is a debt rather than a
marker. The rule lives in one place — isNewAt() for PHP, restrictToNew() for
SQL — and every decision point goes through it: row badge, category tabs,
sidebar dots, counts endpoint, every repository query. NewMailMarkers now
memoises a clock reading alongside its counts so one request cannot straddle
the boundary, and reset() clears it, which under FrankenPHP worker mode is the
line that stops a long-lived worker answering with yesterday's "now".

An aged-out thread keeps listed_at NULL and simply fails the window test. The
column records WHEN A ROW WAS PUT IN FRONT OF THE USER; stamping it for mail
nobody saw writes a false statement that the rethread backfill COALESCEs
forward and any later "was this displayed" feature inherits. A range predicate
on last_message_at — already the list's sort column — is the cheaper mistake.

SEARCH RETIRES THE BADGE. A row whose sender and subject were just read in a
result list has been shown. Rather than a second copy of the ordering, the
collect-render-then-mark path moved to ThreadListRenderer and both controllers
call it, so search inherits the prefetch guard too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Paul LütznerandClaude Opus 5 committed 2026-08-12 21:53:22 +02:00
1 parent c7ec3b36f9
commit 00dfad61aa
13 files changed
+1089 -181
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

assets/controllers/mail/new_marker_controller.js
+99
View File
@@ -0,0 +1,99 @@
import { Controller } from "@hotwired/stimulus";
/**
* Tells the server which "New" rows actually reached the screen.
*
* The server marks a thread listed when it renders one, which is right until
* the render and the display come apart — and Turbo 8 pulls them apart on
* purpose. It prefetches a link on hover, and then serves THAT RESPONSE for the
* click. MailController declines to retire badges on a prefetch, correctly,
* because a hover is not a visit; but when the click reuses the prefetched
* response there is no second request, so nothing ever retired them. Every list
* reached from the sidebar or the category tabs kept its badges through
* navigation and reload, permanently. This controller closes that hole from the
* only place that can actually see a screen.
*
* On the container, not on each row: a page of fifty is one POST.
*
* Rows report once. Reporting is idempotent server-side — the UPDATE only
* touches rows still null — but the `#reported` set keeps an untouched list
* from re-POSTing the same ids every time anything on the page mutates.
*/
export default class extends Controller {
static values = {
url: String,
// Enough for a fragment swap to settle, short enough that a user who
// navigates straight back out has still been counted as having looked.
delay: { type: Number, default: 250 },
};
#reported = new Set();
#timer = null;
#observer = null;
// A field rather than a private method, because the same reference has to
// come back off removeEventListener on disconnect.
#onVisibility = () => {
if (!document.hidden) this.#schedule();
};
connect() {
document.addEventListener("visibilitychange", this.#onVisibility);
// The list frame is swapped in place by the pane controller's refresh
// and by Turbo frame navigation, neither of which re-connects this
// controller — so the rows are watched rather than merely read once.
this.#observer = new MutationObserver(() => this.#schedule());
this.#observer.observe(this.element, { childList: true, subtree: true });
this.#schedule();
}
disconnect() {
clearTimeout(this.#timer);
this.#observer?.disconnect();
document.removeEventListener("visibilitychange", this.#onVisibility);
}
#schedule() {
clearTimeout(this.#timer);
this.#timer = setTimeout(() => this.#report(), this.delayValue);
}
async #report() {
// A list rendered into a background tab is in the document but has not
// been put in front of anybody. Wait for the tab to be looked at — the
// visibilitychange listener above brings us back.
if (document.hidden) return;
const ids = [];
for (const row of this.element.querySelectorAll("[data-new-id]")) {
const id = Number(row.dataset.newId);
if (Number.isInteger(id) && id > 0 && !this.#reported.has(id)) {
this.#reported.add(id);
ids.push(id);
}
}
if (ids.length === 0) return;
try {
await fetch(this.urlValue, {
method: "POST",
headers: {
"Content-Type": "application/json",
"X-CSRF-Token":
document.querySelector('meta[name="csrf-token"]')?.content ?? "",
},
body: JSON.stringify({ ids }),
});
} catch {
// A failed report costs a badge one extra appearance, which is the
// harmless direction. Un-remember the ids so the next mutation or
// the next visit tries again.
for (const id of ids) this.#reported.delete(id);
}
}
}
src/Controller/Mail/MailController.php
+48 -66
View File
@@ -15,6 +15,7 @@ use App\Repository\Mail\AccountRepository;
use App\Repository\Mail\MailboxRepository;
use App\Repository\Mail\MessageRepository;
use App\Repository\Mail\MessageThreadRepository;
use App\Service\Mail\ThreadListRenderer;
use App\Twig\NewMailMarkers;
use App\Twig\SidebarCounts;
use Doctrine\ORM\EntityManagerInterface;
@@ -34,6 +35,7 @@ final class MailController extends AbstractController
private readonly MessageThreadRepository $threadRepository,
private readonly LabelRepository $labelRepository,
private readonly AccountRepository $accountRepository,
private readonly ThreadListRenderer $listRenderer,
)
{
}
@@ -119,86 +121,66 @@ final class MailController extends AbstractController
}
/**
* Render a thread list, then retire the "New" badges it just showed.
* Render a thread list through the shared collect-render-then-mark path.
*
* The order is the feature. A thread is new until its row has been
* DISPLAYED — not until it has been opened — so the marking has to happen
* against a page that has already been turned into HTML. Marking first
* would mean the badge was computed from state the same request had just
* destroyed, and no user would ever see it: mail would arrive and quietly
* become old in the frame that announced it.
*
* $this->render() returns a Response whose body is already a finished
* string, so "after render" is literally true here rather than a hopeful
* reading of the call order.
*
* Scoped to $threads, which is one page of PER_PAGE rows. Marking the whole
* query instead would clear page 2 and 3 the moment page 1 was looked at,
* which is the same bug as the migration's missing backfill, only slower.
*
* Every list view goes through here — including the ones where "new" is a
* strange thing to say, like Sent. A row that has been shown has been
* shown, whichever list showed it, and a view that rendered without marking
* would leave a badge the user has already looked at waiting to surprise
* them somewhere else.
*
* The same call serves the X-List-Fragment refresh, and deliberately: that
* refresh swaps the rendered rows straight into the visible list, so it is
* a display like any other. The refresh is already held while the tab is
* hidden and while a write is in flight — see mail_pane_controller — which
* is what keeps a background poll from retiring badges nobody saw.
* A thin forward to ThreadListRenderer, which SearchController uses too —
* search results are rows the user has been shown like any others.
*
* @param MessageThread[] $threads
* @param array<string, mixed> $parameters
*/
private function renderList(Request $request, string $template, array $threads, array $parameters): Response
{
$new = [];
foreach ($threads as $thread) {
if (null === $thread->listedAt) {
$new[] = (int) $thread->id;
}
}
$parameters['threads'] = $threads;
$parameters['newThreadIds'] = $new;
$response = $this->render($template, $parameters);
// A prefetch is a page nobody has seen. Turbo 8 fetches a link on hover
// — every sidebar row here is prefetched, since only the thread rows
// and the account folder rows opt out — so without this, running the
// pointer across the nav on the way to somewhere else would silently
// retire the badges in Promotions, in Starred and in every label it
// passed over. The rendered HTML is thrown away if the click never
// comes, and the marking would not have been.
if (true === self::isPrefetch($request)) {
return $response;
}
$this->threadRepository->markListed($new, new \DateTimeImmutable());
return $response;
return $this->listRenderer->render($request, $template, $threads, $parameters);
}
/**
* Whether this request is a speculative fetch rather than a visit.
* "These rows are on screen" — the client half of the marker.
*
* Both spellings, because there are two mechanisms: Turbo sends its own
* `X-Sec-Purpose` on hover prefetches, and browsers send the standard
* `Sec-Purpose` for prerender and speculation-rules prefetching. Either one
* means the same thing to this feature — the page may never be looked at.
* This exists because the server cannot tell a render from a display, and
* one particular gap between the two made the badge permanent. Turbo 8
* prefetches links on hover and then reuses that response for the click, so
* every visit to a list reached from the sidebar or the category tabs
* arrived carrying `X-Sec-Purpose: prefetch`. renderList() correctly
* declined to mark a speculative fetch; nothing ever marked the visit,
* because there was no second request to mark it on. The badge survived
* navigation, reload and everything else, forever.
*
* So the DOM reports in. Rows that actually entered the document say so,
* which is the statement the feature has always claimed to make, and the
* one no response header can make on their behalf.
*
* Idempotent by construction: markListedForUser() only writes where
* listedAt is still null, so the ordinary non-prefetched case — already
* marked server-side during the render — costs one UPDATE matching no rows.
*/
private static function isPrefetch(Request $request): bool
#[Route('/threads/listed', name: 'threads_listed', methods: ['POST'])]
public function markListed(Request $request): JsonResponse
{
foreach (['X-Sec-Purpose', 'Sec-Purpose'] as $header) {
if (str_contains((string) $request->headers->get($header), 'prefetch')) {
return true;
}
}
$this->denyAccessUnlessGranted('ROLE_USER');
return false;
$body = json_decode((string) $request->getContent(), true);
$ids = \is_array($body) && \is_array($body['ids'] ?? null) ? $body['ids'] : [];
// Ints only, and capped. The cap is not a security boundary — ownership
// is — but a page renders PER_PAGE rows, so a body an order of
// magnitude past that is not a list anybody looked at.
$ids = \array_slice(
\array_values(\array_filter(\array_map(
static fn (mixed $id): int => (int) $id,
$ids,
), static fn (int $id): bool => $id > 0)),
0,
self::PER_PAGE * 2,
);
$marked = $this->threadRepository->markListedForUser(
$ids,
$this->getUser(),
new \DateTimeImmutable(),
);
return new JsonResponse(['marked' => $marked]);
}
#[Route('/inbox', name: 'inbox')]
src/Controller/Mail/SearchController.php
+9 -2
View File
@@ -7,6 +7,7 @@ namespace App\Controller\Mail;
use App\Domain\Enum\Mail\SearchSortOrder;
use App\Entity\User\User;
use App\Repository\Mail\MessageThreadRepository;
use App\Service\Mail\ThreadListRenderer;
use App\Service\Search\SearchQueryParser;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -23,6 +24,7 @@ final class SearchController extends AbstractController
private readonly SearchQueryParser $parser,
private readonly MessageThreadRepository $threadRepository,
private readonly EntityManagerInterface $em,
private readonly ThreadListRenderer $listRenderer,
) {}
#[Route('', name: '', methods: ['GET'])]
@@ -66,9 +68,14 @@ final class SearchController extends AbstractController
$threads = $this->threadRepository->search($user, $parsed, $page, sort: $sort);
$total = $this->threadRepository->countSearch($user, $parsed);
return $this->render('search/search.html.twig', [
// Through ThreadListRenderer, not $this->render(): a row whose subject
// and sender the user has just read in a result list has been SHOWN,
// and leaving it badged would mean finding your own search results
// announced as news the next time you opened the inbox. Same
// collect-render-then-mark order, same prefetch guard, one
// implementation — see the service.
return $this->listRenderer->render($request, 'search/search.html.twig', $threads, [
'q' => $raw,
'threads' => $threads,
'total' => $total,
'page' => $page,
'per_page' => 50,
src/Entity/Mail/MessageThread.php
+46
View File
@@ -109,6 +109,52 @@ class MessageThread
#[ORM\Column(nullable: true)]
public ?\DateTimeImmutable $listedAt = null;
/**
* How long mail stays "new" on age alone.
*
* A marker you can only clear by looking at every row is not a marker, it
* is a debt — and mail that arrived yesterday is not news whether or not
* anyone scrolled past it. So newness has a second, unconditional exit:
* the window closes on its own.
*
* Spelled once, here, beside the column it qualifies. Every place that
* decides newness — the row badge, the category tabs, the sidebar dots,
* the counts endpoint and every repository query — goes through
* newSince()/isNewAt() rather than repeating "24 hours" in SQL.
*/
public const string NEW_WINDOW = 'PT24H';
/**
* The arrival time a thread has to beat to still count as new.
*
* A static so the repository can put the same boundary in a WHERE clause
* that this entity applies in PHP; if the two ever disagreed, a sidebar dot
* would promise new mail that the list below it declined to badge.
*/
public static function newSince(\DateTimeImmutable $now): \DateTimeImmutable
{
return $now->sub(new \DateInterval(self::NEW_WINDOW));
}
/**
* Is this conversation new AS OF $now — never shown, and recent enough to
* still be worth announcing?
*
* Both halves are required. `listedAt IS NULL` alone was the original rule
* and is what made the badge a chore; the window alone would re-badge mail
* the user has already been shown for the rest of the day.
*
* A thread with no lastMessageAt cannot be shown to have arrived inside the
* window, so it is not new. That errs towards silence, which is the right
* direction for a marker whose whole job is to be believed.
*/
public function isNewAt(\DateTimeImmutable $now): bool
{
return null === $this->listedAt
&& null !== $this->lastMessageAt
&& $this->lastMessageAt >= self::newSince($now);
}
public function __construct()
{
$this->messages = new ArrayCollection();
src/Repository/Mail/MessageThreadRepository.php
+72 -8
View File
@@ -576,6 +576,69 @@ class MessageThreadRepository extends ServiceEntityRepository
->execute();
}
/**
* The same retirement, for ids that arrived from the browser.
*
* markListed() trusts its caller because its caller just read the threads
* out of the database itself. This one is reachable by anybody who can POST
* a JSON array, so ownership is a WHERE clause rather than an assumption:
* without it, a list of guessed ids would let one account retire another
* account's badges.
*
* A subselect on Account rather than a join, because DQL UPDATE cannot
* join — and an id-by-id load-and-check would be one SELECT per row for a
* request that fires on every list view.
*
* @param list<int> $threadIds
*
* @return int rows actually retired
*/
public function markListedForUser(array $threadIds, UserInterface $user, \DateTimeImmutable $at): int
{
if ([] === $threadIds) {
return 0;
}
return (int) $this->createQueryBuilder('t')
->update()
->set('t.listedAt', ':at')
->where('t.id IN (:ids)')
->andWhere('t.listedAt IS NULL')
->andWhere(
't.account IN (SELECT owned.id FROM ' . Account::class . ' owned WHERE owned.usr = :user)',
)
->setParameter('at', $at)
->setParameter('ids', $threadIds)
->setParameter('user', $user)
->getQuery()
->execute();
}
/**
* The "is new" predicate, in one place.
*
* Never shown AND arrived inside MessageThread::NEW_WINDOW. Every count
* below calls this instead of writing `t.listedAt IS NULL` itself, because
* the counts feed dots and the dots have to agree with the badges the list
* renders — and the list decides with MessageThread::isNewAt(). One rule,
* two languages; this is the seam where they are kept identical.
*
* An aged-out thread keeps listedAt NULL rather than being stamped as
* shown: the column says WHEN THE ROW WAS PUT IN FRONT OF THE USER, and
* writing a timestamp there for mail nobody ever saw would be a false
* statement recorded permanently — one the rethread backfill COALESCEs
* forward, and one any later feature reading "was this displayed" would
* inherit. The cost is a range predicate on last_message_at, which is
* already the list's sort column.
*/
private function restrictToNew(QueryBuilder $qb, \DateTimeImmutable $now): void
{
$qb->andWhere('t.listedAt IS NULL')
->andWhere('t.lastMessageAt IS NOT NULL')
->andWhere('t.lastMessageAt >= :newSince')
->setParameter('newSince', MessageThread::newSince($now));
}
/**
* New threads per inbox category — what puts the dot on a Gmail tab.
*
@@ -585,7 +648,7 @@ class MessageThreadRepository extends ServiceEntityRepository
*
* @return array<string,int>
*/
public function countNewByCategoryForUnifiedInbox(UserInterface $user): array
public function countNewByCategoryForUnifiedInbox(UserInterface $user, \DateTimeImmutable $now): array
{
$qb = $this->createQueryBuilder('t')
->select('t.category AS category', 'COUNT(DISTINCT t.id) AS newCount')
@@ -594,11 +657,11 @@ class MessageThreadRepository extends ServiceEntityRepository
->where('a.usr = :user')
->andWhere('a.isActive = true')
->andWhere('l.role = :inbox')
->andWhere('t.listedAt IS NULL')
->groupBy('t.category')
->setParameter('user', $user)
->setParameter('inbox', LabelRole::Inbox);
$this->restrictToNew($qb, $now);
$this->excludeTrashed($qb);
$counts = [];
@@ -629,7 +692,7 @@ class MessageThreadRepository extends ServiceEntityRepository
*
* @return array<string,int> role value → new thread count
*/
public function countNewPerRole(UserInterface $user): array
public function countNewPerRole(UserInterface $user, \DateTimeImmutable $now): array
{
$qb = $this->createQueryBuilder('t')
->select('l.role AS role', 'COUNT(DISTINCT t.id) AS newCount')
@@ -638,10 +701,11 @@ class MessageThreadRepository extends ServiceEntityRepository
->where('a.usr = :user')
->andWhere('a.isActive = true')
->andWhere('l.role IS NOT NULL')
->andWhere('t.listedAt IS NULL')
->setParameter('user', $user)
->groupBy('l.role');
$this->restrictToNew($qb, $now);
$qb->andWhere(
$qb->expr()->orX(
'l.role = :trashRole',
@@ -674,7 +738,7 @@ class MessageThreadRepository extends ServiceEntityRepository
*
* @return array<int,int> label id → new thread count
*/
public function countNewPerUserLabel(UserInterface $user, ?Account $account = null): array
public function countNewPerUserLabel(UserInterface $user, \DateTimeImmutable $now, ?Account $account = null): array
{
$qb = $this->createQueryBuilder('t')
->select('l.id AS labelId', 'COUNT(DISTINCT t.id) AS newCount')
@@ -683,10 +747,10 @@ class MessageThreadRepository extends ServiceEntityRepository
->where('a.usr = :user')
->andWhere('a.isActive = true')
->andWhere('l.role IS NULL')
->andWhere('t.listedAt IS NULL')
->setParameter('user', $user)
->groupBy('l.id');
$this->restrictToNew($qb, $now);
$this->narrowToAccount($qb, $account);
$this->excludeTrashed($qb);
@@ -700,7 +764,7 @@ class MessageThreadRepository extends ServiceEntityRepository
}
/** New threads among the starred ones. */
public function countNewForStarred(UserInterface $user): int
public function countNewForStarred(UserInterface $user, \DateTimeImmutable $now): int
{
$qb = $this->createQueryBuilder('t')
->select('COUNT(DISTINCT t.id)')
@@ -708,9 +772,9 @@ class MessageThreadRepository extends ServiceEntityRepository
->where('a.usr = :user')
->andWhere('a.isActive = true')
->andWhere('t.starredAt IS NOT NULL')
->andWhere('t.listedAt IS NULL')
->setParameter('user', $user);
$this->restrictToNew($qb, $now);
$this->excludeTrashed($qb);
return (int) $qb->getQuery()->getSingleScalarResult();
src/Service/Mail/ThreadListRenderer.php
+98
View File
@@ -0,0 +1,98 @@
<?php
declare(strict_types=1);
namespace App\Service\Mail;
use App\Entity\Mail\MessageThread;
use App\Repository\Mail\MessageThreadRepository;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Twig\Environment;
/**
* Render a thread list, then retire the "New" badges it just showed.
*
* A service rather than a method on MailController because SearchController
* shows thread rows too, and a row the user has read the subject line of in a
* search result has been shown — it must not be waiting to surprise them in the
* inbox afterwards. Duplicating the collect-render-then-mark order into a second
* controller is how the two would drift; there is one copy of it here.
*
* The ORDER is the feature. A thread is new until its row has been DISPLAYED —
* not until it has been opened — so the marking has to happen against a page
* that has already been turned into HTML. Marking first would mean the badge
* was computed from state the same request had just destroyed, and no user
* would ever see it: mail would arrive and quietly become old in the frame that
* announced it.
*
* Scoped to the $threads handed in, which is one rendered page. Marking the
* whole query instead would clear page 2 and 3 the moment page 1 was looked at.
*
* WHAT THIS CANNOT DO, and why there is a client half as well: the server knows
* it produced HTML, never that the HTML reached a screen. Turbo 8 prefetches
* links on hover and then SERVES THAT EXACT RESPONSE for the click, so a page
* rendered under a prefetch header is very often the page the user ends up
* looking at. Skipping the mark here was therefore not conservative, it was
* permanent — see NewMailMarkerController on the client, which confirms the
* display from the DOM and is what actually retires a prefetched list.
*/
final class ThreadListRenderer
{
public function __construct(
private readonly Environment $twig,
private readonly MessageThreadRepository $threadRepository,
) {
}
/**
* @param MessageThread[] $threads
* @param array<string, mixed> $parameters
*/
public function render(Request $request, string $template, array $threads, array $parameters): Response
{
$now = new \DateTimeImmutable();
$new = [];
foreach ($threads as $thread) {
if (true === $thread->isNewAt($now)) {
$new[] = (int) $thread->id;
}
}
$parameters['threads'] = $threads;
$parameters['newThreadIds'] = $new;
$response = new Response($this->twig->render($template, $parameters));
// A prefetch is a page that MAY never be looked at, so the server does
// not retire anything on one. The client confirms the display instead
// — which also covers the case this guard used to break, where the
// prefetched response is the one that gets shown.
if (true === self::isPrefetch($request)) {
return $response;
}
$this->threadRepository->markListed($new, $now);
return $response;
}
/**
* Whether this request is a speculative fetch rather than a visit.
*
* Both spellings, because there are two mechanisms: Turbo sends its own
* `X-Sec-Purpose` on hover prefetches, and browsers send the standard
* `Sec-Purpose` for prerender and speculation-rules prefetching.
*/
public static function isPrefetch(Request $request): bool
{
foreach (['X-Sec-Purpose', 'Sec-Purpose'] as $header) {
if (str_contains((string) $request->headers->get($header), 'prefetch')) {
return true;
}
}
return false;
}
}
src/Twig/NewMailMarkers.php
+44 -6
View File
@@ -7,6 +7,7 @@ namespace App\Twig;
use App\Domain\Enum\Mail\LabelRole;
use App\Domain\Enum\Mail\MessageCategory;
use App\Entity\Label\Label;
use App\Entity\Mail\MessageThread;
use App\Repository\Mail\MessageThreadRepository;
use Symfony\Bundle\SecurityBundle\Security;
use Symfony\Contracts\Service\ResetInterface;
@@ -15,8 +16,10 @@ use Symfony\Contracts\Service\ResetInterface;
* Where new mail is sitting — the numbers behind every "something arrived here"
* dot.
*
* New is `MessageThread::$listedAt IS NULL`: the conversation has never had its
* row put in front of the user. Not unread; see the property's own note.
* New is MessageThread::isNewAt(): the conversation has never had its row put in
* front of the user AND it arrived inside MessageThread::NEW_WINDOW. Not unread;
* see the property's own note. The window is why this class now holds a clock
* reading as well as a set of counts.
*
* A sibling of SidebarCounts rather than a set of methods on it, because the
* two answer different questions with the same shape and merging them would
@@ -41,6 +44,17 @@ class NewMailMarkers implements ResetInterface
private ?int $starred = null;
/**
* The instant this request calls "now".
*
* Held for the length of one request so the five category dots, the sidebar
* dots and the row badges all answer against a single clock reading. Two
* reads a few milliseconds apart could otherwise straddle the 24-hour
* boundary and put a dot on a category whose only new thread the list below
* had just declined to badge.
*/
private ?\DateTimeImmutable $now = null;
public function __construct(
private readonly MessageThreadRepository $threadRepository,
private readonly Security $security,
@@ -52,6 +66,12 @@ class NewMailMarkers implements ResetInterface
* LogAlertGlobal::reset(). FrankenPHP keeps this service alive between
* requests, so without this the first user a worker serves decides where
* every later user sees a dot.
*
* $now is cleared here too, and that line is now the load-bearing one.
* Newness became time-dependent when it grew a window, so a worker that
* kept the first request's clock reading would go on answering "new" for
* mail that aged out hours ago — and unlike the per-user counts, that fault
* survives even a worker only ever serving one person.
*/
public function reset(): void
{
@@ -59,6 +79,24 @@ class NewMailMarkers implements ResetInterface
$this->byRole = null;
$this->byLabel = null;
$this->starred = null;
$this->now = null;
}
/**
* Whether a thread is new, for the templates.
*
* The single-row counterpart of the counts below, and deliberately reading
* the same clock: a row redrawn on its own by a turbo-stream asks this, the
* dots ask the counts, and both have to be talking about the same instant.
*/
public function isNew(MessageThread $thread): bool
{
return $thread->isNewAt($this->now());
}
private function now(): \DateTimeImmutable
{
return $this->now ??= new \DateTimeImmutable();
}
public function forCategory(MessageCategory $category): int
@@ -68,7 +106,7 @@ class NewMailMarkers implements ResetInterface
$this->byCategory = null === $user
? []
: $this->threadRepository->countNewByCategoryForUnifiedInbox($user);
: $this->threadRepository->countNewByCategoryForUnifiedInbox($user, $this->now());
}
return $this->byCategory[$category->value] ?? 0;
@@ -81,7 +119,7 @@ class NewMailMarkers implements ResetInterface
$this->byRole = null === $user
? []
: $this->threadRepository->countNewPerRole($user);
: $this->threadRepository->countNewPerRole($user, $this->now());
}
return $this->byRole[$role->value] ?? 0;
@@ -94,7 +132,7 @@ class NewMailMarkers implements ResetInterface
$this->byLabel = null === $user
? []
: $this->threadRepository->countNewPerUserLabel($user);
: $this->threadRepository->countNewPerUserLabel($user, $this->now());
}
return $this->byLabel[(int) $label->id] ?? 0;
@@ -107,7 +145,7 @@ class NewMailMarkers implements ResetInterface
$this->starred = null === $user
? 0
: $this->threadRepository->countNewForStarred($user);
: $this->threadRepository->countNewForStarred($user, $this->now());
}
return $this->starred;
templates/_layout/_mailbox.html.twig
+9
View File
@@ -112,6 +112,15 @@ Pagination variables expected from the controller (all optional, fallback to 0/1
id="message-list"
data-sync-scope="{% block list_scope %}*{% endblock %}"
data-mail--mail-pane-target="list"
{# Confirms to the server that the rows below reached the
screen. Mounted on the container rather than on each
row so fifty rows are one report, and here rather than
inside the frame so it survives a fragment swap and
watches for one. #}
{% if app.user %}
data-controller="mail--new-marker"
data-mail--new-marker-url-value="{{ path('app_mail_threads_listed') }}"
{% endif %}
{# @container, so the rows below switch to their stacked
layout on the LIST's width. They used to switch on the
viewport's, which is a different number entirely once
templates/_partials/_thread_row.html.twig
+6 -1
View File
@@ -55,7 +55,7 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
{% set isNew = isThread
and (newThreadIds is defined
? item.id in newThreadIds
: item.listedAt is null) %}
: newMail.isNew(item)) %}
{% if isThread %}
{% set type = 'thread' %}
@@ -145,6 +145,11 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
are orthogonal, and a row that is `new="false" unread="true"` — scrolled
past, never opened — is the case the whole feature exists to express. #}
data-new="{{ isNew ? 'true' : 'false' }}"
{# The id the client reports back once this row is actually in the document,
so a list Turbo served from its prefetch cache still retires its badges —
see mail--new-marker and MailController::markListed(). Only on rows that
are claiming to be new; there is nothing to report about the others. #}
{% if isNew %}data-new-id="{{ rowId }}"{% endif %}
data-starred="{{ starred ? 'true' : 'false' }}"
>
{# Full-row clickable overlay #}
tests/Controller/Mail/BadgeSemanticsTest.php
+130
View File
@@ -5,8 +5,12 @@ declare(strict_types=1);
namespace App\Tests\Controller\Mail;
use App\Domain\Enum\Mail\LabelRole;
use App\Domain\Enum\Mail\MessageCategory;
use App\Repository\User\UserRepository;
use App\Tests\Support\Mail\SeedsMarkerFixtures;
use App\Twig\SidebarCounts;
use Doctrine\DBAL\Connection;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\KernelBrowser;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
@@ -33,6 +37,8 @@ use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
*/
final class BadgeSemanticsTest extends WebTestCase
{
use SeedsMarkerFixtures;
private const string ADMIN_EMAIL = 'e2e-admin@plmail.test';
private function signedIn(): KernelBrowser
@@ -211,4 +217,128 @@ final class BadgeSemanticsTest extends WebTestCase
self::assertNotContains($key, $badgeKeys);
}
}
// ── the timeout, where the two could most easily disagree ─────────────
/**
* Newness now has a 24-hour ceiling, and a ceiling applied in one place and
* not the other is exactly the 188-vs-193 fault this file exists for — only
* worse, because a dot carries no number to argue with. A sidebar that
* says "something arrived here" over a list that badges nothing sends the
* user looking for mail that is not there.
*
* So: a mailbox holding one thread on each side of the window, and the
* count endpoint asked to agree with the rows actually rendered.
*
* Against a fixture mailbox of its own rather than the shared admin seed,
* because "one in, one out" is the whole point and cannot be arranged in a
* mailbox whose contents this test did not choose.
*/
public function testTheNewCountsHonourTheTimeoutThatTheBadgesDo(): void
{
$client = $this->fixtureClient();
$this->thread('arrived just now');
$this->thread('arrived the day before yesterday', lastMessageAt: 'now -40 hours');
$client->request('GET', '/mail/inbox');
self::assertResponseIsSuccessful();
$badged = $client->getCrawler()->filter('[data-thread-new]')->count();
self::assertSame(1, $badged, 'only the recent one is news');
// Read BEFORE the render that retires it would be ideal, but the render
// above has already marked what it showed — so this asks the endpoint
// the same question the next page load would, and both must say zero.
$counts = $this->countsFrom($client);
self::assertSame(
0,
$counts['new:category:' . MessageCategory::Primary->value],
'the shown thread is retired and the old one aged out, so nothing is left to dot',
);
}
/**
* The agreement that matters most, stated as an invariant: the number of
* "New" badges the inbox renders is the number the endpoint would put on
* the Primary tab, at the moment before the render retires them.
*/
public function testTheCategoryDotCountMatchesTheBadgesTheListRenders(): void
{
$client = $this->fixtureClient();
$this->thread('recent one');
$this->thread('recent two');
$this->thread('far too old', lastMessageAt: 'now -3 days');
// The counts endpoint renders no list, so it retires nothing — this is
// the state the inbox is about to draw from.
$expected = $this->countsFrom($client)['new:category:' . MessageCategory::Primary->value];
$client->request('GET', '/mail/inbox');
$badged = $client->getCrawler()->filter('[data-thread-new]')->count();
self::assertSame(2, $expected, 'two inside the window, one aged out');
self::assertSame($expected, $badged, 'the dot must count exactly what the list badges');
}
/**
* A client signed in as a user whose whole mailbox this test wrote, inside
* a transaction that tearDown rolls back.
*/
private function fixtureClient(): KernelBrowser
{
$client = static::createClient();
// Without this the kernel is rebuilt between requests, taking the
// connection holding the transaction with it, and the fixtures vanish
// before the second request can see them.
$client->disableReboot();
$container = static::getContainer();
$this->em = $container->get(EntityManagerInterface::class);
$this->connection = $container->get(Connection::class);
$this->connection->beginTransaction();
$this->user = $this->seedUser();
$this->account = $this->seedAccount();
$this->inbox = $this->seedLabel('Inbox', LabelRole::Inbox);
$client->loginUser($this->user);
return $client;
}
/** @return array<string,int> */
private function countsFrom(KernelBrowser $client): array
{
$client->request('GET', '/mail/sidebar/counts');
self::assertResponseIsSuccessful();
/** @var array<string,int> */
return json_decode(
(string) $client->getResponse()->getContent(),
true,
flags: JSON_THROW_ON_ERROR,
);
}
private ?Connection $connection = null;
protected function tearDown(): void
{
if (null !== $this->connection && true === $this->connection->isTransactionActive()) {
$this->connection->rollBack();
}
$this->connection = null;
parent::tearDown();
}
}
tests/Controller/Mail/NewMailMarkerTest.php
+235 -97
View File
@@ -18,6 +18,7 @@ use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\Version\Version;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\KernelBrowser;
use App\Tests\Support\Mail\SeedsMarkerFixtures;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
use Symfony\Component\DomCrawler\Crawler;
@@ -49,15 +50,13 @@ use Symfony\Component\DomCrawler\Crawler;
*/
final class NewMailMarkerTest extends WebTestCase
{
use SeedsMarkerFixtures;
/** The migration under test — spelled once, and read back by two tests. */
private const string MIGRATION = 'DoctrineMigrations\Version20260812161500';
private KernelBrowser $client;
private EntityManagerInterface $em;
private Connection $connection;
private User $user;
private Account $account;
private Label $inbox;
protected function setUp(): void
{
@@ -246,7 +245,9 @@ final class NewMailMarkerTest extends WebTestCase
for ($i = 0; $i <= 50; ++$i) {
$threads[] = $this->thread(
sprintf('thread %02d', $i),
lastMessageAt: sprintf('2026-01-01 09:00 +%d hours', $i),
// Inside the window, and ascending, so thread 0 is the oldest
// and lands on page two.
lastMessageAt: sprintf('now -%d minutes', 200 - $i),
flush: false,
);
}
@@ -440,7 +441,235 @@ final class NewMailMarkerTest extends WebTestCase
self::assertSame('false', $this->rowAttribute($thread, 'data-new'));
}
// ── helpers ───────────────────────────────────────────────────────────
// ── the window: "new" times out ──────────────────────────────────────
/**
* The user's second complaint, and the more interesting one: "mail thats a
* day old isnt new. you dont have to see EVERY mail to get rid of new
* tags."
*
* That is a statement about what the marker is FOR. As first built, the
* badge could only be cleared by having its row rendered, which makes it a
* debt the user works off one page at a time — a backlog of unvisited
* labels stays badged forever. A "new" marker is supposed to say "this
* arrived while you were away", and after a day that is no longer an
* interesting thing to say about anything.
*
* Both edges, because a window asserted on one side is a window that can
* silently become "always" or "never".
*/
public function testMailInsideTheWindowIsStillNew(): void
{
$thread = $this->thread('23 hours old', lastMessageAt: 'now -23 hours');
$this->client->request('GET', '/mail/inbox');
self::assertSame(
'true',
$this->rowAttribute($thread, 'data-new'),
'inside the window, an unshown thread is still news',
);
}
public function testMailPastTheWindowIsNoLongerNew(): void
{
$thread = $this->thread('25 hours old', lastMessageAt: 'now -25 hours');
$this->client->request('GET', '/mail/inbox');
self::assertSame(
'false',
$this->rowAttribute($thread, 'data-new'),
'a day-old conversation is not new, however few rows have been looked at',
);
}
/**
* The window is an age test, NOT a second way of stamping listedAt.
*
* An aged-out thread keeps listedAt null on purpose. The column records
* WHEN THE ROW WAS PUT IN FRONT OF THE USER, and stamping it for mail
* nobody ever saw would write a false statement that outlives the marker:
* the rethread backfill COALESCEs listedAt forward across a rebuild, and
* any later feature asking "was this ever displayed" would inherit the lie.
* Failing the window test is cheap by comparison — one range predicate on
* last_message_at, which is already the list's sort column.
*/
public function testAgingOutDoesNotStampTheThreadAsHavingBeenShown(): void
{
$thread = $this->thread('long past it', lastMessageAt: 'now -3 days');
$this->client->request('GET', '/mail/inbox');
self::assertSame('false', $this->rowAttribute($thread, 'data-new'));
self::assertTrue(
$this->isNew($thread),
'listedAt must stay null — nobody was shown this row',
);
}
/** The dots time out with the badges, or they promise mail the list denies. */
public function testAnAgedOutThreadStopsDottingItsPlace(): void
{
$this->thread('yesterdays promo', category: MessageCategory::Promotions, lastMessageAt: 'now -30 hours');
$this->thread('a normal one');
$this->client->request('GET', '/mail/inbox');
self::assertFalse(
$this->dotIsVisible('new:category:' . MessageCategory::Promotions->value),
'Promotions holds nothing recent, so it must not claim to',
);
$counts = $this->countsPayload();
self::assertSame(0, $counts['new:category:' . MessageCategory::Promotions->value]);
}
// ── search ───────────────────────────────────────────────────────────
/**
* "search can remove new tags."
*
* A row whose subject and sender the user has just read in a result list
* has been shown, whatever route put it there. Leaving it badged means
* finding your own search results announced back to you as news.
*/
public function testSearchResultsRetireTheBadge(): void
{
$thread = $this->thread('quarterly invoice');
$this->client->request('GET', '/mail/search?q=quarterly');
self::assertResponseIsSuccessful();
self::assertSame(
'true',
$this->rowAttribute($thread, 'data-new'),
'the result list still shows the badge in the frame that retires it',
);
self::assertFalse($this->isNew($thread), 'and search counts as having been shown');
}
/** Search takes the same path, so it inherits the same prefetch guard. */
public function testAPrefetchedSearchRetiresNothing(): void
{
$thread = $this->thread('quarterly invoice');
$this->client->request(
'GET',
'/mail/search?q=quarterly',
server: ['HTTP_X_SEC_PURPOSE' => 'prefetch'],
);
self::assertResponseIsSuccessful();
self::assertTrue($this->isNew($thread));
}
// ── the client's confirmation: the reload bug ────────────────────────
/**
* The bug the user actually hit: "the tag survived a reload and tab
* navigation."
*
* Turbo 8 prefetches a link on hover and then SERVES THAT RESPONSE for the
* click, so every list reached from the sidebar or the tab strip arrives
* carrying X-Sec-Purpose: prefetch and there is no second request. The
* server was right to refuse to mark a speculative fetch and wrong to
* assume another request would follow; nothing ever retired the badge.
*
* The fix is that the DOM reports the rows it is actually showing. This is
* the controller half of that — see new-mail-marker.spec.ts for the half
* that proves a real browser does it.
*/
public function testRowsReportedByTheClientAreRetired(): void
{
$thread = $this->thread('arrived by prefetch');
// Exactly what a hovered sidebar link does: the page renders, and
// nothing is marked.
$this->client->request('GET', '/mail/inbox', server: ['HTTP_X_SEC_PURPOSE' => 'prefetch']);
self::assertTrue($this->isNew($thread));
$this->client->request(
'POST',
'/mail/threads/listed',
server: ['CONTENT_TYPE' => 'application/json'],
content: json_encode(['ids' => [$thread->id]], JSON_THROW_ON_ERROR),
);
self::assertResponseIsSuccessful();
self::assertFalse($this->isNew($thread), 'a row the browser says it showed is a row that was shown');
}
/**
* The endpoint takes ids straight off the wire, so ownership is a WHERE
* clause rather than an assumption. Without it a guessed id retires
* somebody else's badge.
*/
public function testTheClientCannotRetireAnotherUsersBadges(): void
{
$mine = $this->thread('mine');
$stranger = $this->seedUser();
$strangersAccount = new Account();
$strangersAccount->usr = $stranger;
$strangersAccount->name = 'Someone else';
$strangersAccount->email = uniqid('other-', true) . '@example.test';
$strangersAccount->username = uniqid('other-', true);
$strangersAccount->imapHost = 'imap.example.test';
$strangersAccount->imapPort = 993;
$strangersAccount->imapEncryption = 'ssl';
$strangersAccount->authType = 'password';
$strangersAccount->isActive = true;
$this->em->persist($strangersAccount);
$strangersThread = new MessageThread();
$strangersThread->account = $strangersAccount;
$strangersThread->subject = 'not yours';
$strangersThread->normalizedSubject = 'not yours';
$strangersThread->threadingMethod = ThreadingMethod::SubjectFallback;
$strangersThread->lastMessageAt = new DateTimeImmutable();
$strangersThread->category = MessageCategory::Primary;
$strangersThread->messageCount = 1;
$strangersThread->unreadCount = 0;
$this->em->persist($strangersThread);
$this->em->flush();
$this->client->request(
'POST',
'/mail/threads/listed',
server: ['CONTENT_TYPE' => 'application/json'],
content: json_encode(
['ids' => [$mine->id, $strangersThread->id]],
JSON_THROW_ON_ERROR,
),
);
self::assertResponseIsSuccessful();
self::assertFalse($this->isNew($mine));
self::assertTrue(
$this->isNew($strangersThread),
'another account\'s badge is not ours to retire',
);
}
/** Garbage in the body is not a 500. */
public function testTheEndpointShrugsOffARubbishBody(): void
{
$this->client->request(
'POST',
'/mail/threads/listed',
server: ['CONTENT_TYPE' => 'application/json'],
content: 'not json at all',
);
self::assertResponseIsSuccessful();
}
// ── helpers ──────────────────────────────────────────────────────────
/**
* The statements the migration would run.
@@ -520,95 +749,4 @@ final class NewMailMarkerTest extends WebTestCase
flags: JSON_THROW_ON_ERROR,
);
}
// ── fixtures ──────────────────────────────────────────────────────────
private function thread(
string $subject,
?MessageCategory $category = null,
string $lastMessageAt = '2026-03-01 09:00',
int $unread = 0,
bool $flush = true,
): MessageThread {
$thread = new MessageThread();
$thread->account = $this->account;
$thread->subject = $subject;
$thread->normalizedSubject = mb_strtolower($subject);
$thread->threadingMethod = ThreadingMethod::SubjectFallback;
$thread->lastMessageAt = new DateTimeImmutable($lastMessageAt);
$thread->category = $category ?? MessageCategory::Primary;
$thread->messageCount = 1;
$thread->unreadCount = $unread;
$thread->addLabel($this->inbox);
$this->em->persist($thread);
$message = new Message();
$message->account = $this->account;
$message->thread = $thread;
$message->subject = $subject;
$message->fromAddress = 'sender@example.test';
$message->receivedAt = new DateTimeImmutable($lastMessageAt);
$message->sentAt = $message->receivedAt;
$message->seenAt = $unread > 0 ? null : new DateTimeImmutable($lastMessageAt);
$message->flags = [];
$message->hasAttachments = false;
$thread->addMessage($message);
$this->em->persist($message);
if (true === $flush) {
$this->em->flush();
}
return $thread;
}
private function seedLabel(string $name, ?LabelRole $role = null): Label
{
$label = new Label();
$label->usr = $this->user;
$label->name = $name;
$label->role = $role;
$label->isVisible = true;
$this->em->persist($label);
$this->em->flush();
return $label;
}
private function seedAccount(): Account
{
$account = new Account();
$account->usr = $this->user;
$account->name = 'Marker fixture';
$account->email = uniqid('marker-', true) . '@example.test';
$account->username = uniqid('marker-', true);
$account->imapHost = 'imap.example.test';
$account->imapPort = 993;
$account->imapEncryption = 'ssl';
$account->authType = 'password';
$account->isActive = true;
$this->em->persist($account);
$this->em->flush();
return $account;
}
private function seedUser(): User
{
$user = new User();
$user->email = 'marker-' . uniqid('', true) . '@example.test';
$user->nameFirst = 'Marker';
$user->nameLast = 'Fixture';
$user->roles = ['ROLE_USER'];
$user->password = 'x';
$this->em->persist($user);
$this->em->flush();
return $user;
}
}
tests/Support/Mail/SeedsMarkerFixtures.php
+135
View File
@@ -0,0 +1,135 @@
<?php
declare(strict_types=1);
namespace App\Tests\Support\Mail;
use App\Domain\Enum\Mail\LabelRole;
use App\Domain\Enum\Mail\MessageCategory;
use App\Domain\Enum\Mail\ThreadingMethod;
use App\Entity\Label\Label;
use App\Entity\Mail\Account;
use App\Entity\Mail\Message;
use App\Entity\Mail\MessageThread;
use App\Entity\User\User;
use DateTimeImmutable;
use Doctrine\ORM\EntityManagerInterface;
/**
* A user, an account, an Inbox label and threads of a chosen age.
*
* Extracted from NewMailMarkerTest once BadgeSemanticsTest needed the same
* fixtures: the dots-agree-with-the-badges assertion is only worth anything
* against a mailbox whose contents the test chose, and in particular against
* one holding mail on both sides of the 24-hour window. Two copies of this
* would be two definitions of "a new thread", which is precisely the drift
* those tests exist to catch.
*
* The consumer owns $em, $user, $account and $inbox, and is expected to be
* running inside a transaction it rolls back.
*/
trait SeedsMarkerFixtures
{
private EntityManagerInterface $em;
private User $user;
private Account $account;
private Label $inbox;
/**
* One conversation, as old as you say.
*
* $lastMessageAt is relative by default and must stay that way. Newness has
* a 24-hour ceiling (MessageThread::NEW_WINDOW), so a fixture pinned to a
* calendar date silently becomes aged-out mail as soon as the wall clock
* passes it — which is how "just arrived" fixtures written as '2026-03-01'
* would quietly stop testing the badge at all.
*/
private function thread(
string $subject,
?MessageCategory $category = null,
string $lastMessageAt = 'now',
int $unread = 0,
bool $flush = true,
): MessageThread {
$thread = new MessageThread();
$thread->account = $this->account;
$thread->subject = $subject;
$thread->normalizedSubject = mb_strtolower($subject);
$thread->threadingMethod = ThreadingMethod::SubjectFallback;
$thread->lastMessageAt = new DateTimeImmutable($lastMessageAt);
$thread->category = $category ?? MessageCategory::Primary;
$thread->messageCount = 1;
$thread->unreadCount = $unread;
$thread->addLabel($this->inbox);
$this->em->persist($thread);
$message = new Message();
$message->account = $this->account;
$message->thread = $thread;
$message->subject = $subject;
$message->fromAddress = 'sender@example.test';
$message->receivedAt = new DateTimeImmutable($lastMessageAt);
$message->sentAt = $message->receivedAt;
$message->seenAt = $unread > 0 ? null : new DateTimeImmutable($lastMessageAt);
$message->flags = [];
$message->hasAttachments = false;
$thread->addMessage($message);
$this->em->persist($message);
if (true === $flush) {
$this->em->flush();
}
return $thread;
}
private function seedLabel(string $name, ?LabelRole $role = null): Label
{
$label = new Label();
$label->usr = $this->user;
$label->name = $name;
$label->role = $role;
$label->isVisible = true;
$this->em->persist($label);
$this->em->flush();
return $label;
}
private function seedAccount(): Account
{
$account = new Account();
$account->usr = $this->user;
$account->name = 'Marker fixture';
$account->email = uniqid('marker-', true) . '@example.test';
$account->username = uniqid('marker-', true);
$account->imapHost = 'imap.example.test';
$account->imapPort = 993;
$account->imapEncryption = 'ssl';
$account->authType = 'password';
$account->isActive = true;
$this->em->persist($account);
$this->em->flush();
return $account;
}
private function seedUser(): User
{
$user = new User();
$user->email = 'marker-' . uniqid('', true) . '@example.test';
$user->nameFirst = 'Marker';
$user->nameLast = 'Fixture';
$user->roles = ['ROLE_USER'];
$user->password = 'x';
$this->em->persist($user);
$this->em->flush();
return $user;
}
}
tests/e2e/new-mail-marker.spec.ts
+158 -1
View File
@@ -1,5 +1,6 @@
import { test, expect } from "./support/test";
import { INBOX_SUBJECTS, mailRow, seed } from "./support/config";
import { INBOX_SUBJECTS, TEST_USER, mailRow, seed } from "./support/config";
import { execSync } from "node:child_process";
/**
* The "New" badge, end to end: it appears when mail arrives and is gone the
@@ -120,3 +121,159 @@ test.describe("the new-mail marker", () => {
await expect(dot).toBeHidden();
});
});
/**
* What the database says about a subject, straight out of Postgres.
*
* Asserting on pixels alone is what let the original bug ship: every spec here
* navigated with page.goto(), which asks the server fresh every time, so
* "badge gone" and "row marked listed" were indistinguishable. They are not the
* same statement, and the tests below need to be able to tell them apart.
*/
function isListed(subject: string): boolean {
const compose = process.env.E2E_COMPOSE ?? "docker compose -f compose.test.yaml";
// Scoped to THIS worker's user, not just the subject. Every parallel slot
// seeds the same four subjects into a mailbox of its own, so a bare
// `where subject = …` reads another worker's rows and answers about mail
// this test never touched — which it did, intermittently, at workers > 1.
const out = execSync(
`${compose} exec -T database psql -U app -d app_test -t -A -c ` +
`"select count(*) from message_thread t ` +
`join account a on a.id = t.account_id ` +
`join \\"user\\" u on u.id = a.usr_id ` +
`where u.email = '${TEST_USER.email}' ` +
`and t.subject = '${subject}' and t.listed_at is not null"`,
{ encoding: "utf8" },
).trim();
return Number(out) > 0;
}
/**
* The reported bug: "when i tried the 'new' feature the tag survived a reload
* and tab navigation. i dont think thats correct."
*
* It did, and nothing in the suite caught it, because nothing in the suite
* navigated the way a person does. page.goto() is a browser load: Turbo is not
* involved, no link is hovered, and the request reaches the server as an
* ordinary visit that the server duly marks. Clicking a link is a different
* mechanism entirely — Turbo 8 prefetches on hover, the server refuses to
* retire badges on a prefetch (rightly: a hover is not a visit), and then Turbo
* SERVES THAT PREFETCHED RESPONSE for the click. There is no second request to
* mark. The badge was never retired, on any navigation, ever.
*
* So these specs click. That is the whole point of them, and any rewrite that
* turns a click back into a goto puts the bug back beyond reach of the suite.
*/
test.describe("the badge and real navigation", () => {
test("a list reached by CLICKING a nav link still retires its badges", async ({
page,
}) => {
// Land somewhere that is not the inbox, so the inbox rows are untouched.
await page.goto("/mail/starred");
expect(
isListed(INBOX_SUBJECTS.read),
"precondition: the seeded inbox has not been shown yet",
).toBe(false);
const inboxLink = page.locator('#sidebar a[href="/mail/inbox"]').first();
// Hover first, and let the prefetch actually fire. Without this the
// click is a plain fetch and the test proves nothing about the bug.
await inboxLink.hover();
await page.waitForTimeout(500);
expect(
isListed(INBOX_SUBJECTS.read),
"hovering is not looking — a prefetch must never retire a badge",
).toBe(false);
await inboxLink.click();
const row = mailRow(page, INBOX_SUBJECTS.read);
await expect(row).toHaveAttribute("data-new", "true");
// The client confirms the display; the badge is retired for real.
await expect
.poll(() => isListed(INBOX_SUBJECTS.read), {
message: "the rows reached the screen, so the server has to be told",
timeout: 5_000,
})
.toBe(true);
});
test("the badge does not survive a reload", async ({ page }) => {
await page.goto("/mail/starred");
const inboxLink = page.locator('#sidebar a[href="/mail/inbox"]').first();
await inboxLink.hover();
await page.waitForTimeout(500);
await inboxLink.click();
await expect(mailRow(page, INBOX_SUBJECTS.read)).toHaveAttribute("data-new", "true");
await expect.poll(() => isListed(INBOX_SUBJECTS.read), { timeout: 5_000 }).toBe(true);
await page.reload();
const reloaded = mailRow(page, INBOX_SUBJECTS.read);
await expect(reloaded).toHaveAttribute("data-new", "false");
await expect(reloaded.locator(NEW_BADGE)).toHaveCount(0);
});
test("the badge does not survive navigating away and back", async ({ page }) => {
await page.goto("/mail/starred");
const inboxLink = page.locator('#sidebar a[href="/mail/inbox"]').first();
const starredLink = page.locator('#sidebar a[href="/mail/starred"]').first();
await inboxLink.hover();
await page.waitForTimeout(500);
await inboxLink.click();
await expect(mailRow(page, INBOX_SUBJECTS.read)).toHaveAttribute("data-new", "true");
await expect.poll(() => isListed(INBOX_SUBJECTS.read), { timeout: 5_000 }).toBe(true);
await starredLink.hover();
await starredLink.click();
await expect(page.locator("#message-list")).toBeVisible();
// Back to the inbox the same way it was reached the first time — hover,
// prefetch, click. This is the exact sequence that used to redraw the
// badge indefinitely.
await inboxLink.hover();
await page.waitForTimeout(500);
await inboxLink.click();
const second = mailRow(page, INBOX_SUBJECTS.read);
await expect(second).toHaveAttribute("data-new", "false");
await expect(second.locator(NEW_BADGE)).toHaveCount(0);
});
});
/**
* "search can remove 'new' tags."
*
* A row read in a result list has been shown, and search now goes through the
* same collect-render-then-mark path as every other list.
*/
test.describe("search", () => {
test("finding a conversation in search retires its badge", async ({ page }) => {
await page.goto(`/mail/search?q=${encodeURIComponent(INBOX_SUBJECTS.read)}`);
const result = mailRow(page, INBOX_SUBJECTS.read);
await expect(result).toBeVisible();
await expect(result).toHaveAttribute("data-new", "true");
await expect
.poll(() => isListed(INBOX_SUBJECTS.read), { timeout: 5_000 })
.toBe(true);
await page.goto("/mail/inbox");
const inInbox = mailRow(page, INBOX_SUBJECTS.read);
await expect(inInbox).toHaveAttribute("data-new", "false");
await expect(inInbox.locator(NEW_BADGE)).toHaveCount(0);
});
});