From 44c77b268d13acd0ac5c891cb873d8a21359264b Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Fri, 25 Sep 2026 19:27:24 +0200 Subject: [PATCH] fix: keep the server's order for sender and subject sorted lists A sender or subject order is collated by the server. Stalwart 1.0 ranks byte-order sort keys (lowercased name and address, the thread name for the subject) and puts empty values last in both directions. The client re-sorted every page and every thread group with `localeCompare` and empty values first, which turned that order around. Pages and thread groups whose order has a sender or subject level now keep the query's order (the Email/get list is put back into the query's id order). Other orders keep the client-side safety sort, and the unified view still merges accounts with the client comparator. --- lib/__tests__/jmap-list-order.test.ts | 59 ++++++++++++++++++++++++ lib/__tests__/message-list-order.test.ts | 16 +++++++ lib/__tests__/thread-utils.test.ts | 24 ++++++++++ lib/jmap/client.ts | 19 ++++++-- lib/message-list-order.ts | 18 +++++++- lib/thread-utils.ts | 10 +++- 6 files changed, 139 insertions(+), 7 deletions(-) diff --git a/lib/__tests__/jmap-list-order.test.ts b/lib/__tests__/jmap-list-order.test.ts index ab6f2f24b..e8c633af2 100644 --- a/lib/__tests__/jmap-list-order.test.ts +++ b/lib/__tests__/jmap-list-order.test.ts @@ -191,4 +191,63 @@ describe('JMAPClient list ordering', () => { [{ property: 'receivedAt', isAscending: false }], ]); }); + + describe('sender and subject orders keep the server order', () => { + const sender = (id: string, name: string, address: string, receivedAt: string) => ({ + id, threadId: `t-${id}`, keywords: { $seen: true }, receivedAt, mailboxIds: {}, size: 1, hasAttachment: false, + from: address ? [{ name, email: address }] : [], + }); + const zoe = sender('zoe', 'Zoe', 'zoe@example.org', '2026-01-01T00:00:00Z'); + const emile = sender('emile', 'Émile', 'emile@example.org', '2026-01-02T00:00:00Z'); + const nobody = sender('nobody', '', '', '2026-01-03T00:00:00Z'); + + /** A folder query answered in `queryOrder`, with Email/get listing the objects in `getOrder`. */ + function replyInOrder(queryOrder: typeof zoe[], getOrder: typeof zoe[]) { + return (methodCalls: MethodCall[]) => ({ + methodResponses: [ + ['Email/query', { ids: queryOrder.map((e) => e.id), total: queryOrder.length }, methodCalls[0][2]], + ['Email/get', { list: getOrder }, methodCalls[1][2]], + ], + }); + } + + it('keeps Stalwart 1.0 byte order with the empty sender last', async () => { + // 1.0 ranks lowercased "name address" keys by bytes, so "zoe ..." sorts + // before "émile ..." and a message without a sender comes last. The + // client comparator (localeCompare, empty first) would reverse all three. + const client = await connectedClient(); + const sent = recordRequests(replyInOrder([zoe, emile, nobody], [nobody, emile, zoe])); + + const { emails } = await client.getEmails('inbox', undefined, 50, 0, undefined, false, undefined, [{ criterion: 'from', direction: 'asc' }]); + + expect(folderQueries(sent)).toEqual([[ + { property: 'from', isAscending: true }, + { property: 'receivedAt', isAscending: false }, + ]]); + expect(emails.map((e) => e.id)).toEqual(['zoe', 'emile', 'nobody']); + }); + + it('keeps whatever order Stalwart 0.16 returns when a subject level follows another', async () => { + // 0.16 leaves the collation to its search store. + const client = await connectedClient(); + recordRequests(replyInOrder([emile, nobody, zoe], [zoe, nobody, emile])); + + const { emails } = await client.getEmails('inbox', undefined, 50, 0, undefined, false, undefined, [ + { criterion: 'size', direction: 'desc' }, + { criterion: 'subject', direction: 'asc' }, + ]); + + expect(emails.map((e) => e.id)).toEqual(['emile', 'nobody', 'zoe']); + }); + + it('still re-sorts a page ordered without text levels', async () => { + // The safety net for servers that ignore the sort stays for everything else. + const client = await connectedClient(); + recordRequests(replyInOrder([zoe, emile, nobody], [zoe, emile, nobody])); + + const { emails } = await client.getEmails('inbox', undefined, 50, 0, undefined, false, undefined, [{ criterion: 'receivedAt', direction: 'desc' }]); + + expect(emails.map((e) => e.id)).toEqual(['nobody', 'emile', 'zoe']); + }); + }); }); diff --git a/lib/__tests__/message-list-order.test.ts b/lib/__tests__/message-list-order.test.ts index be105efdc..86f3cc683 100644 --- a/lib/__tests__/message-list-order.test.ts +++ b/lib/__tests__/message-list-order.test.ts @@ -4,6 +4,7 @@ import { compareEmails, detectPreset, hasKeywordLevels, + hasTextLevels, isDefaultOrder, keywordFirst, levelKeyword, @@ -103,6 +104,21 @@ describe('keyword semantics', () => { }); }); +describe('hasTextLevels', () => { + // Only the server knows the collation of a sender or subject order. + it('is true for any order with a sender or subject level', () => { + expect(hasTextLevels([{ criterion: 'from', direction: 'asc' }])).toBe(true); + expect(hasTextLevels([{ criterion: 'subject', direction: 'desc' }])).toBe(true); + expect(hasTextLevels([...UNREAD_FIRST, { criterion: 'from', direction: 'asc' }])).toBe(true); + }); + + it('is false for date, size and keyword orders', () => { + expect(hasTextLevels([])).toBe(false); + expect(hasTextLevels([...UNREAD_FIRST, ...STARRED_FIRST, { criterion: 'size', direction: 'desc' }])).toBe(false); + expect(hasTextLevels([{ criterion: 'receivedAt', direction: 'asc' }, { criterion: 'sentAt', direction: 'desc' }])).toBe(false); + }); +}); + describe('buildEmailSort', () => { it('is plain newest-first with no levels', () => { expect(buildEmailSort([])).toEqual([{ property: 'receivedAt', isAscending: false }]); diff --git a/lib/__tests__/thread-utils.test.ts b/lib/__tests__/thread-utils.test.ts index ea1acbd5a..4f2fb08bc 100644 --- a/lib/__tests__/thread-utils.test.ts +++ b/lib/__tests__/thread-utils.test.ts @@ -193,6 +193,30 @@ describe('sortThreadGroups', () => { ]; expect(groupEmailsByThread(emails)[0].hasPinned).toBe(true); }); + + it('keeps the fetched order of a sender order, as Stalwart 1.0 collates it', () => { + // The server's byte order puts "zoe" before "émile" and the message + // without a sender last; a localeCompare re-sort would turn that around. + const fetched = [ + makeEmail({ id: 'z', threadId: 'tz', from: [{ name: 'Zoe', email: 'zoe@example.org' }] }), + makeEmail({ id: 'e1', threadId: 'te', from: [{ name: 'Émile', email: 'emile@example.org' }] }), + makeEmail({ id: 'n', threadId: 'tn', from: [] }), + makeEmail({ id: 'e2', threadId: 'te', from: [{ name: 'Émile', email: 'emile@example.org' }], receivedAt: '2024-02-01T00:00:00Z' }), + ]; + const order = [{ criterion: 'from' as const, direction: 'asc' as const }]; + expect(sortThreadGroups(groupEmailsByThread(fetched), order).map(g => g.threadId)).toEqual(['tz', 'te', 'tn']); + }); + + it('still lifts pinned threads above a subject order, and otherwise keeps the fetched order', () => { + // Same subject everywhere: a client re-sort would fall back to newest first. + const groups = [ + makeGroup('first', '2024-01-01T00:00:00Z'), + makeGroup('second', '2024-06-01T00:00:00Z'), + makeGroup('pinned', '2024-03-01T00:00:00Z', true), + ]; + const sorted = sortThreadGroups(groups, [{ criterion: 'subject', direction: 'asc' }]); + expect(sorted.map(g => g.threadId)).toEqual(['pinned', 'first', 'second']); + }); }); describe('getThreadParticipants', () => { diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index c877665f5..e4bb3bd66 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -26,7 +26,7 @@ import { acceptedFileName, fileNameRulesFrom, type FileNameRules } from "@/lib/f import { folderDepthError } from "@/lib/file-errors"; import { mediaTypeEssence, OCTET_STREAM } from "@/lib/media-type"; import { getEffectiveTimeZone, toLocalDateTime } from "@/lib/timezone"; -import { buildEmailSort, compareEmails, hasKeywordLevels, type KeywordSortPolarity, type SortLevel } from "@/lib/message-list-order"; +import { buildEmailSort, compareEmails, hasKeywordLevels, hasTextLevels, type KeywordSortPolarity, type SortLevel } from "@/lib/message-list-order"; import type { AccountInfo } from "./types"; import { stalwartChangeId } from "./stalwart-state"; @@ -2147,10 +2147,19 @@ export class JMAPClient implements IJMAPClient { if (response.methodResponses?.[1]?.[0] === "Email/get" && getResponse) { const emails = (getResponse.list || []) as Email[]; - // Sort client-side as safety net - some servers may not honour - // the query sort for large mailboxes without additional filters. - // Must mirror the query sort, or it would undo the configured order. - emails.sort(compareEmails(order, { pinnedFirst })); + if (hasTextLevels(order)) { + // A sender or subject order is the server's collation, which the + // client comparator contradicts (Stalwart 1.0 compares byte-order + // sort keys, empty values last). Keep the query's order: /get may + // return the objects in any order (RFC 8620 section 5.1). + const rank = new Map(((queryResponse?.ids ?? []) as string[]).map((id, i) => [id, i])); + emails.sort((a, b) => (rank.get(a.id) ?? rank.size) - (rank.get(b.id) ?? rank.size)); + } else { + // Sort client-side as safety net - some servers may not honour + // the query sort for large mailboxes without additional filters. + // Must mirror the query sort, or it would undo the configured order. + emails.sort(compareEmails(order, { pinnedFirst })); + } const total = queryResponse?.total || 0; const hasMore = computeHasMore(position, emails.length, total, limit); diff --git a/lib/message-list-order.ts b/lib/message-list-order.ts index ea19dc1e7..1a660f007 100644 --- a/lib/message-list-order.ts +++ b/lib/message-list-order.ts @@ -149,6 +149,20 @@ export function hasKeywordLevels(levels: SortLevel[], pinnedFirst = false): bool return pinnedFirst || levels.some(l => levelKeyword(l) !== null); } +// Sender and subject orders are collated by the server. Stalwart 1.0 compares +// byte-order sort keys (lowercased name and address, the thread name for the +// subject) and puts empty values last in both directions; 0.16 leaves it to +// its search store. `compareEmails` approximates neither. +const TEXT_CRITERIA: ReadonlySet = new Set(['from', 'subject']); + +/** + * True when the order compares text: the list keeps the order the server + * returned instead of being re-sorted on the client. + */ +export function hasTextLevels(levels: SortLevel[]): boolean { + return levels.some(l => TEXT_CRITERIA.has(l.criterion)); +} + /** A "chronological" order: nothing configured, or only newest-first. */ export function isDefaultOrder(levels: SortLevel[]): boolean { return levels.length === 0 @@ -244,7 +258,9 @@ function compareValues(a: number | string, b: number | string): number { * Client-side mirror of `buildEmailSort` for the same level list. Used as the * within-page safety net (some servers ignore the sort on unfiltered queries), * for merging per-account pages in the unified view, for the demo client, and - * to keep thread grouping from undoing the server order. + * to keep thread grouping from undoing the server order. Sender and subject + * levels are an approximation (see `hasTextLevels`), so a single-account list + * with one keeps the server's order. */ export function compareEmails( levels: SortLevel[], diff --git a/lib/thread-utils.ts b/lib/thread-utils.ts index bb78a4eaf..c77182674 100644 --- a/lib/thread-utils.ts +++ b/lib/thread-utils.ts @@ -1,5 +1,5 @@ import type { Email, ThreadGroup } from "./jmap/types"; -import { compareEmails, type SortLevel } from "./message-list-order"; +import { compareEmails, hasTextLevels, type SortLevel } from "./message-list-order"; /** * Client-side identity of a thread. @@ -109,8 +109,16 @@ export function groupEmailsByThread( * takes the position of whichever of its emails sorts first under `order` * (RFC 8621 §4.4.3 thread collapsing semantics) - with the default order that * is the latest email's receivedAt date, newest first. + * + * A sender or subject order only the server can apply (see `hasTextLevels`). + * The groups arrive in fetched order - groupEmailsByThread keeps the order in + * which each thread first appears - so each thread already sits where its + * first-ranked email does, and only pinned threads move. */ export function sortThreadGroups(groups: ThreadGroup[], order: SortLevel[] = []): ThreadGroup[] { + if (hasTextLevels(order)) { + return [...groups].sort((a, b) => (b.hasPinned ? 1 : 0) - (a.hasPinned ? 1 : 0)); + } const compare = compareEmails(order); const representative = new Map(); for (const group of groups) {