thunderbird-patch-review
Easy-to-use patch review interface for Thunderbird
Contribute: ~marcc/thunderbird-review-plugin@lists.sr.ht
git clone git://mccd.space/thunderbird-patch-review
| Log | Files | Refs | README | LICENSE |
commit 8de066f9af393dd265ee29e5237821432ba3b49d parent 9f6dc91cab6f837c81fe27f7a04acad9d22bc35c Author: Pi Agent <agent@pi.local> Date: Sat, 18 Jul 2026 13:30:05 +0200 series: prefer the diff-bearing message when an index collides A reply that shares the patch subject sometimes loses its Re: prefix on the way through a mailing list, so isReplySubject does not catch it and sameSeries pulls it in. The per-index dedup then kept the later message by date, so the reply (no diff) displaced the real patch and the review tab showed "No diff found in this message" for a slot that does have a patch. Fetch bodies before dedup and, for n > 0, prefer the entry whose body has a diff; only tiebreak on date. Extracted as dedupByIndex so the choice is unit-testable. Diffstat:
| M | extension/modules/series.js | | | 57 | +++++++++++++++++++++++++++++++++++++++++++++------------ |
| M | tests/run.mjs | | | 38 | ++++++++++++++++++++++++++++++++++++++ |
2 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/extension/modules/series.js b/extension/modules/series.js
@@ -53,6 +53,37 @@ async function listFolderMessages(anchor, windowDays) {
}
/**
+ * Collapse a list of same-series entries to one per series index.
+ *
+ * Each entry is { header, info, body, hasDiff } (only header.date,
+ * header.headerMessageId, info.n and hasDiff are read here).
+ *
+ * For a real patch slot (n > 0) a body that carries a diff always beats one
+ * that does not — a reply that shares the subject (some mailing lists strip
+ * the "Re:" prefix) would otherwise win by date and surface as
+ * "No diff found in this message". With no diff tiebreak, the more recent
+ * message wins (handles resends of the same index).
+ */
+export function dedupByIndex(entries) {
+ const byIndex = new Map();
+ for (const entry of entries) {
+ const n = entry.info ? entry.info.n : null;
+ const key = n === null ? `id:${entry.header.headerMessageId}` : n;
+ const prev = byIndex.get(key);
+ if (!prev) {
+ byIndex.set(key, entry);
+ continue;
+ }
+ if (n !== null && n > 0 && entry.hasDiff !== prev.hasDiff) {
+ byIndex.set(key, entry.hasDiff ? entry : prev);
+ } else {
+ byIndex.set(key, entry.header.date > prev.header.date ? entry : prev);
+ }
+ }
+ return byIndex;
+}
+
+/**
* Build the ordered series for the given message header.
* @returns {Promise<Array<{
* id: number, headerMessageId: string, subject: string, author: string,
@@ -93,17 +124,20 @@ export async function collectSeries(anchor) {
}
}
- // Keep one message per series index (a resend may duplicate an index).
- const byIndex = new Map();
- for (const entry of members.values()) {
- const n = entry.info ? entry.info.n : null;
- const key = n === null ? `id:${entry.header.headerMessageId}` : n;
- const prev = byIndex.get(key);
- if (!prev || entry.header.date > prev.header.date) {
- byIndex.set(key, entry);
- }
+ // Fetch bodies up front so the per-index dedup can prefer the message
+ // that actually carries a diff. A reply that shares the subject (some
+ // mailing lists strip the "Re:" prefix, and isReplySubject only catches
+ // the ones that keep it) would otherwise win the slot by date and show up
+ // as "No diff found in this message".
+ const entries = [];
+ for (const { header, info } of members.values()) {
+ const body = await getPlainBody(header.id);
+ entries.push({ header, info, body, hasDiff: bodyLooksLikeDiff(body) });
}
+ // Keep one message per series index (a resend may duplicate an index).
+ const byIndex = dedupByIndex(entries);
+
const ordered = [...byIndex.values()].sort((a, b) => {
const an = a.info && a.info.n !== null ? a.info.n : 0;
const bn = b.info && b.info.n !== null ? b.info.n : 0;
@@ -111,8 +145,7 @@ export async function collectSeries(anchor) {
});
const series = [];
- for (const { header, info } of ordered) {
- const body = await getPlainBody(header.id);
+ for (const { header, info, body, hasDiff } of ordered) {
series.push({
id: header.id,
headerMessageId: header.headerMessageId,
@@ -124,7 +157,7 @@ export async function collectSeries(anchor) {
isCover: Boolean(info && info.n === 0),
title: info ? info.title : header.subject,
body,
- hasDiff: bodyLooksLikeDiff(body),
+ hasDiff,
});
}
return series;
diff --git a/tests/run.mjs b/tests/run.mjs
@@ -15,6 +15,7 @@ import { parsePatchEmail } from "../extension/modules/diff-parse.js";
import { pairRanges, computeIntraline } from "../extension/modules/intraline.js";
import { formatReview } from "../extension/modules/reply-format.js";
import { locator, GENERAL } from "../extension/modules/review-store.js";
+import { dedupByIndex } from "../extension/modules/series.js";
import {
isSourcehutList,
SOURCEHUT_STATUSES,
@@ -73,6 +74,43 @@ function check(name, cond, extra = "") {
check("isPatchMessage: plain mail", !isPatchMessage("hello", "how are you?\n"));
}
+// --- series dedup ----------------------------------------------------------
+
+{
+ const mk = (n, date, hasDiff, id = `m${date}`) => ({
+ header: { headerMessageId: id, date },
+ info: n === null ? null : { n },
+ body: "",
+ hasDiff,
+ });
+
+ // A reply that shares the index but carries no diff must lose to the real
+ // patch, even when the reply is newer (simulates a list that stripped Re:).
+ const d = dedupByIndex([
+ mk(1, 1000, true, "patch1"),
+ mk(1, 2000, false, "reply1"), // newer, no diff
+ ]);
+ check("dedup: diff beats newer no-diff for n>0", d.get(1).header.headerMessageId === "patch1");
+
+ // Two real patches (resend) with diffs: keep the newer one.
+ const d2 = dedupByIndex([
+ mk(2, 1000, true, "patch2v1"),
+ mk(2, 2000, true, "patch2v2"),
+ ]);
+ check("dedup: newer diff wins among diffs", d2.get(2).header.headerMessageId === "patch2v2");
+
+ // Cover letter (n=0): no diff on either, prefer the newer cover.
+ const d3 = dedupByIndex([
+ mk(0, 1000, false, "cover1"),
+ mk(0, 2000, false, "cover2"),
+ ]);
+ check("dedup: cover keeps newer when no diff on either", d3.get(0).header.headerMessageId === "cover2");
+
+ // Lone n (null index) keyed by message id does not collide.
+ const d4 = dedupByIndex([mk(null, 1000, true, "solo")]);
+ check("dedup: null-index keyed by id", d4.get("id:solo").header.headerMessageId === "solo");
+}
+
// --- reply detection -------------------------------------------------------
{