From 324fb48312882dfdf2bddf04507e057876a5a8e5 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Tue, 29 Sep 2026 20:28:39 +0200 Subject: [PATCH] fix: keep an opened mail in place in unread-first lists until another is opened --- components/email/email-list.tsx | 7 +- components/mail/mail-app.tsx | 2 + lib/__tests__/message-list-order.test.ts | 38 ++++++++++ lib/__tests__/thread-utils.test.ts | 14 ++++ lib/message-list-order.ts | 44 ++++++++++-- lib/thread-utils.ts | 9 +-- .../email-store-keyword-order-refill.test.ts | 60 +++++++++++++--- stores/email-store.ts | 69 ++++++++++++++++--- 8 files changed, 210 insertions(+), 33 deletions(-) diff --git a/components/email/email-list.tsx b/components/email/email-list.tsx index b4c8f386f..a8c88500c 100644 --- a/components/email/email-list.tsx +++ b/components/email/email-list.tsx @@ -143,12 +143,15 @@ export function EmailList({ // Search results and cross-account views are always chronological. const fetchedListOrder = useEmailStore((state) => state.listOrder); const crossView = useEmailStore((state) => state.crossView); + // The row opened last stays where it was clicked while that order would + // move it (e.g. read in "unread first"). + const listHold = useEmailStore((state) => state.listHold); const threadGroups = useMemo(() => { const listOrder = searchQuery || crossView || !isFilterEmpty(searchFilters) ? [] : fetchedListOrder; const groups = groupEmailsByThread(emails, disableThreading || isScheduledView, threadEmailCounts); - return sortThreadGroups(groups, listOrder); - }, [emails, disableThreading, isScheduledView, threadEmailCounts, fetchedListOrder, searchQuery, crossView, searchFilters]); + return sortThreadGroups(groups, listOrder, listHold?.keywords); + }, [emails, disableThreading, isScheduledView, threadEmailCounts, fetchedListOrder, searchQuery, crossView, searchFilters, listHold]); const { contextMenu, openContextMenu, closeContextMenu, menuRef } = useContextMenu(); /** diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 4d3446c46..38dffd897 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -3525,6 +3525,8 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { const handleOpenConversation = async (thread: ThreadGroup) => { if (!client) return; + // The view marks the thread read; keep its row where it was tapped. + useEmailStore.getState().holdListRow(thread.threadKey); setConversationThread(thread); setIsLoadingConversation(true); setActiveView("viewer"); diff --git a/lib/__tests__/message-list-order.test.ts b/lib/__tests__/message-list-order.test.ts index be105efdc..df9c6e56b 100644 --- a/lib/__tests__/message-list-order.test.ts +++ b/lib/__tests__/message-list-order.test.ts @@ -9,6 +9,7 @@ import { levelKeyword, orderForMailbox, orderKeywords, + placeHeldRows, presetLevels, sanitizeSortLevels, MAX_SORT_LEVELS, @@ -207,6 +208,43 @@ describe('compareEmails', () => { ])); expect(sorted.map(e => e.id)).toEqual(['unread-new', 'starred-old', 'read-newest']); }); + + it('sorts a held mail by its held keywords', () => { + const justRead = makeEmail({ id: 'just-read', receivedAt: '2026-01-04T00:00:00Z', keywords: { $seen: true } }); + const held = new Map([['just-read', {}]]); + const sorted = [unreadOld, readNewest, justRead, unreadNew].sort(compareEmails(UNREAD_FIRST, { held })); + expect(sorted.map(e => e.id)).toEqual(['just-read', 'unread-new', 'unread-old', 'read-newest']); + }); +}); + +describe('placeHeldRows', () => { + const mail = (id: string, day: number, read: boolean, extra: Record = {}) => + makeEmail({ id, receivedAt: `2026-01-0${day}T00:00:00Z`, keywords: read ? { $seen: true, ...extra } : { ...extra } }); + + it('moves a held mail back to where it sorted when it was opened', () => { + // Server order after reading u2: the unread, then the read by date. + const list = [mail('u3', 3, false), mail('u1', 1, false), mail('r4', 4, true), mail('u2', 2, true)]; + const placed = placeHeldRows(list, UNREAD_FIRST, new Map([['u2', {}]])); + expect(placed.map(e => e.id)).toEqual(['u3', 'u2', 'u1', 'r4']); + }); + + it('leaves the other rows in their order', () => { + const list = [mail('u3', 3, false), mail('u1', 1, false), mail('r4', 4, true), mail('r2', 2, true)]; + const placed = placeHeldRows(list, UNREAD_FIRST, new Map([['r4', { $seen: true }]])); + expect(placed.map(e => e.id)).toEqual(['u3', 'u1', 'r4', 'r2']); + }); + + it('keeps pinned mail above a held one', () => { + const list = [mail('p', 1, true, { $pinned: true }), mail('u1', 1, false), mail('r9', 9, true)]; + const placed = placeHeldRows(list, UNREAD_FIRST, new Map([['r9', {}]])); + expect(placed.map(e => e.id)).toEqual(['p', 'r9', 'u1']); + }); + + it('does nothing for an order without keyword levels', () => { + const list = [mail('a', 3, true), mail('b', 2, false)]; + expect(placeHeldRows(list, [], new Map([['a', {}]]))).toBe(list); + expect(placeHeldRows(list, UNREAD_FIRST, undefined)).toBe(list); + }); }); describe('orderForMailbox', () => { diff --git a/lib/__tests__/thread-utils.test.ts b/lib/__tests__/thread-utils.test.ts index 095142a7a..3a7fb8894 100644 --- a/lib/__tests__/thread-utils.test.ts +++ b/lib/__tests__/thread-utils.test.ts @@ -177,6 +177,20 @@ describe('sortThreadGroups', () => { expect(sorted.map(g => g.threadId)).toEqual(['mixed', 'read-newer']); }); + it('keeps a thread read since it was opened where it was', () => { + const opened = makeGroup('opened', '2024-02-01T00:00:00Z'); + opened.emails = [makeEmail({ id: 'o', receivedAt: '2024-02-01T00:00:00Z', keywords: { $seen: true } })]; + const unread = makeGroup('unread', '2024-01-01T00:00:00Z'); + unread.emails = [makeEmail({ id: 'u', receivedAt: '2024-01-01T00:00:00Z', keywords: {} })]; + const readNewer = makeGroup('read-newer', '2024-06-01T00:00:00Z'); + const order = [{ criterion: 'unread' as const, direction: 'desc' as const }]; + + expect(sortThreadGroups([readNewer, unread, opened], order).map(g => g.threadId)) + .toEqual(['unread', 'read-newer', 'opened']); + expect(sortThreadGroups([readNewer, unread, opened], order, new Map([['o', {}]])).map(g => g.threadId)) + .toEqual(['opened', 'unread', 'read-newer']); + }); + it('keeps pinned threads on top of a configured order', () => { const pinnedRead = makeGroup('pinned-read', '2024-01-01T00:00:00Z', true); const unread = makeGroup('unread', '2024-06-01T00:00:00Z'); diff --git a/lib/message-list-order.ts b/lib/message-list-order.ts index ea19dc1e7..41681b8ca 100644 --- a/lib/message-list-order.ts +++ b/lib/message-list-order.ts @@ -220,11 +220,11 @@ function time(value: string | undefined): number { return Number.isNaN(t) ? 0 : t; } -function levelValue(level: SortLevel, email: Email): number | string { +function levelValue(level: SortLevel, email: Email, keywords: Email['keywords'] | undefined): number | string { switch (level.criterion) { - case 'unread': return email.keywords?.$seen ? 0 : 1; - case 'starred': return email.keywords?.$flagged ? 1 : 0; - case 'tag': return level.tagId && email.keywords?.[`$label:${level.tagId}`] ? 1 : 0; + case 'unread': return keywords?.$seen ? 0 : 1; + case 'starred': return keywords?.$flagged ? 1 : 0; + case 'tag': return level.tagId && keywords?.[`$label:${level.tagId}`] ? 1 : 0; case 'receivedAt': return time(email.receivedAt); case 'sentAt': return time(email.sentAt || email.receivedAt); case 'size': return email.size ?? 0; @@ -240,25 +240,37 @@ function compareValues(a: number | string, b: number | string): number { return a - b; } +/** + * Keywords by email id that a list sorts some rows by instead of their current + * ones: the conversation opened last keeps the place it had when it was opened, + * so reading it in an "unread first" list does not move it (see + * EmailStore.listHold). + */ +export type HeldKeywords = ReadonlyMap; + /** * 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. `held` rows sort by + * their held keywords. */ export function compareEmails( levels: SortLevel[], - opts: { pinnedFirst?: boolean } = {}, + opts: { pinnedFirst?: boolean; held?: HeldKeywords } = {}, ): (a: Email, b: Email) => number { const hasDateLevel = levels.some(l => l.criterion === 'receivedAt'); + const held = opts.held; return (a, b) => { if (opts.pinnedFirst) { const pa = a.keywords?.['$pinned'] ? 1 : 0; const pb = b.keywords?.['$pinned'] ? 1 : 0; if (pa !== pb) return pb - pa; } + const ka = held?.get(a.id) ?? a.keywords; + const kb = held?.get(b.id) ?? b.keywords; for (const level of levels) { - const diff = compareValues(levelValue(level, a), levelValue(level, b)); + const diff = compareValues(levelValue(level, a, ka), levelValue(level, b, kb)); if (diff !== 0) return level.direction === 'asc' ? diff : -diff; } if (!hasDateLevel) { @@ -269,6 +281,24 @@ export function compareEmails( }; } +/** + * Moves the held rows of a list in `levels` order to where they sort by their + * held keywords; every other row keeps its place. The list shows them there, + * and next/previous and the pick after a delete walk this array. + */ +export function placeHeldRows(emails: Email[], levels: SortLevel[], held: HeldKeywords | undefined): Email[] { + if (!held || held.size === 0 || orderKeywords(levels).length === 0) return emails; + if (!emails.some((e) => held.has(e.id))) return emails; + const compare = compareEmails(levels, { pinnedFirst: true, held }); + const out = emails.filter((e) => !held.has(e.id)); + for (const row of emails) { + if (!held.has(row.id)) continue; + const at = out.findIndex((e) => compare(row, e) < 0); + out.splice(at === -1 ? out.length : at, 0, row); + } + return out; +} + /** * The order to use for a given folder: the configured levels when they apply * to every folder, or to the Inbox when scoped to it; chronological otherwise. diff --git a/lib/thread-utils.ts b/lib/thread-utils.ts index a3476c6ec..4920c8a9f 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, type HeldKeywords, type SortLevel } from "./message-list-order"; /** * Client-side identity of a thread. @@ -108,10 +108,11 @@ export function groupEmailsByThread( * the server-side pinned-first sort of the email list. Below that, each thread * 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. + * is the latest email's receivedAt date, newest first. `held` emails sort by + * the keywords they had when their conversation was opened. */ -export function sortThreadGroups(groups: ThreadGroup[], order: SortLevel[] = []): ThreadGroup[] { - const compare = compareEmails(order); +export function sortThreadGroups(groups: ThreadGroup[], order: SortLevel[] = [], held?: HeldKeywords): ThreadGroup[] { + const compare = compareEmails(order, { held }); const representative = new Map(); for (const group of groups) { representative.set( diff --git a/stores/__tests__/email-store-keyword-order-refill.test.ts b/stores/__tests__/email-store-keyword-order-refill.test.ts index 7a8d228f4..13cf97167 100644 --- a/stores/__tests__/email-store-keyword-order-refill.test.ts +++ b/stores/__tests__/email-store-keyword-order-refill.test.ts @@ -55,10 +55,11 @@ function fakeServer(rows: Email[]) { return { client, setSeen }; } -/** The mail at the top of the list as the user sees it. */ +/** The first unread mail in the list as the user sees it. */ function topMail(): Email { - const { emails, listOrder } = useEmailStore.getState(); - return [...emails].sort(compareEmails(listOrder))[0]; + const { emails, listOrder, listHold } = useEmailStore.getState(); + const shown = [...emails].sort(compareEmails(listOrder, { held: listHold?.keywords })); + return shown.find(e => !e.keywords.$seen) ?? shown[0]; } async function readTopMail(client: IJMAPClient): Promise { @@ -96,13 +97,13 @@ describe('keyword-ordered folder list refill (#718)', () => { for (let i = 0; i < 5; i++) opened.push((await readTopMail(client)).id); expect(opened).toEqual(['u1', 'u2', 'u3', 'u4', 'u5']); - // All read now: the head of the server order is newest first, plus the - // open mail, which sorts further down. - expect(ids()).toEqual(['u1', 'r1', 'u2', 'u5']); + // All read now: the head of the server order is newest first, below the + // open mail, which stays where it was clicked. + expect(ids()).toEqual(['u5', 'u1', 'r1', 'u2']); expect(useEmailStore.getState().retainedInViewIds).toEqual(new Set(['u5'])); }); - it('keeps the open mail listed after it moved past the loaded rows, and drops it once another is opened', async () => { + it('keeps the open mail where it was clicked, and drops it once another is opened', async () => { const { client } = fakeServer([ mail('r1', 22, true), mail('r2', 21, true), mail('u1', 20, false), mail('u2', 18, false), mail('u3', 17, false), mail('u4', 15, false), @@ -110,10 +111,47 @@ describe('keyword-ordered folder list refill (#718)', () => { await useEmailStore.getState().fetchEmails(client); await readTopMail(client); - expect(ids()).toEqual(['u2', 'u3', 'u4', 'u1']); + expect(ids()).toEqual(['u1', 'u2', 'u3', 'u4']); await readTopMail(client); - expect(ids()).toEqual(['u3', 'u4', 'r1', 'u2']); + expect(ids()).toEqual(['u2', 'u3', 'u4', 'r1']); + }); + + it('keeps the read mail in place after it is closed, until another is opened', async () => { + const { client } = fakeServer([ + mail('u1', 20, false), mail('u2', 18, false), mail('u3', 17, false), mail('u4', 15, false), mail('r1', 22, true), + ]); + await useEmailStore.getState().fetchEmails(client); + await readTopMail(client); + useEmailStore.getState().selectEmail(null); + + await useEmailStore.getState().refreshCurrentMailbox(client); + expect(ids()).toEqual(['u1', 'u2', 'u3', 'u4']); + + useEmailStore.getState().selectEmail(useEmailStore.getState().emails.find(e => e.id === 'u3')!); + await useEmailStore.getState().refreshCurrentMailbox(client); + expect(ids()).toEqual(['u2', 'u3', 'u4']); + }); + + it('holds a thread when it is expanded, before it is marked read', async () => { + const { client } = fakeServer([mail('u1', 20, false), mail('u2', 18, false), mail('u3', 17, false), mail('u4', 15, false)]); + await useEmailStore.getState().fetchEmails(client); + + useEmailStore.getState().toggleThreadExpansion('t-u1'); + + expect(useEmailStore.getState().listHold?.keywords).toEqual(new Map([['u1', {}]])); + }); + + it('forgets the held mail when the folder is loaded again', async () => { + const { client } = fakeServer([mail('u1', 20, false), mail('u2', 18, false), mail('u3', 17, false), mail('u4', 15, false)]); + await useEmailStore.getState().fetchEmails(client); + await readTopMail(client); + expect(useEmailStore.getState().listHold?.rowKey).toBe('t-u1'); + + await useEmailStore.getState().fetchEmails(client); + + expect(useEmailStore.getState().listHold).toBeNull(); + expect(ids()).toEqual(['u2', 'u3', 'u4']); }); it('does not skip a mail when loading more after the open mail moved', async () => { @@ -123,13 +161,13 @@ describe('keyword-ordered folder list refill (#718)', () => { ]); await useEmailStore.getState().fetchEmails(client); await readTopMail(client); - expect(ids()).toEqual(['u2', 'u3', 'u4', 'u1']); + expect(ids()).toEqual(['u1', 'u2', 'u3', 'u4']); await useEmailStore.getState().loadMoreEmails(client); // Server order is now u2 u3 u4 u5 u1 r1 r2: the page starts at u5. expect(client.getEmails.mock.lastCall?.[3]).toBe(3); - expect(ids()).toEqual(['u2', 'u3', 'u4', 'u1', 'u5', 'r1']); + expect(ids()).toEqual(['u1', 'u2', 'u3', 'u4', 'u5', 'r1']); // u1 came back in order with that page. expect(useEmailStore.getState().retainedInViewIds.size).toBe(0); }); diff --git a/stores/email-store.ts b/stores/email-store.ts index 48e1f83d7..2c6bbb4e0 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -4,7 +4,7 @@ import type { UnifiedMailboxRole, CrossView } from "@/lib/jmap/types"; import type { IJMAPClient } from "@/lib/jmap/client-interface"; import { useSettingsStore, getMessageListOrderFor } from "@/stores/settings-store"; import { useCalendarStore } from "@/stores/calendar-store"; -import { orderKeywords, type SortLevel } from "@/lib/message-list-order"; +import { orderKeywords, placeHeldRows, type HeldKeywords, type SortLevel } from "@/lib/message-list-order"; import { SearchFilters, DEFAULT_SEARCH_FILTERS, buildJMAPFilter, isFilterEmpty } from "@/lib/jmap/search-utils"; import { emailHooks } from "@/lib/plugin-hooks"; import { resolveThreadRoute } from "@/lib/thread-routing"; @@ -75,15 +75,20 @@ interface EmailStore { // Emails the user just read (unread view) or unstarred (starred view) that // should stay visible in that self-filtering cross view until it is re-opened, // instead of vanishing on the next push refresh. In a folder sorted on a - // keyword (#718): the open conversation's rows, kept although a read/star/tag - // change moved them past the loaded part of the server order. Cleared on - // navigation. + // keyword (#718): the open conversation's rows and the held ones (listHold), + // kept although a read/star/tag change moved them past the loaded part of + // the server order. Cleared on navigation. retainedInViewIds: Set; hasMoreEmails: boolean; // Track if more emails are available to load totalEmails: number; // Total number of emails in the current mailbox/query // The configured message-list order the current folder view was fetched // with (#718); empty for chronological. Thread grouping mirrors it. listOrder: SortLevel[]; + // The list row opened last and the keywords its messages had then. Its + // messages sort by those, so reading one in an "unread first" list leaves + // the row where it was clicked; opening another row releases it. Cleared + // on navigation. + listHold: ListHold | null; isPushConnected: boolean; // Track if push notifications are connected lastPushUpdate: number | null; // Timestamp of last push update // Delta sync (RFC 8620 §5.2). The Email collection state the plain @@ -194,6 +199,8 @@ interface EmailStore { */ fetchAccountMailboxes: (client: IJMAPClient, accountId: string) => Promise; selectEmail: (email: Email | null) => void; + /** Holds a list row in place (see listHold); `rowKey` as from listRowKey. */ + holdListRow: (rowKey: string) => void; selectMailbox: (mailboxId: string) => void; setLoading: (loading: boolean) => void; setLoadingEmail: (loading: boolean) => void; @@ -491,6 +498,20 @@ function coalesceRefresh(client: IJMAPClient, key: string, run: () => Promise((set, get) => ({ hasMoreEmails: false, totalEmails: 0, listOrder: [], + listHold: null, isPushConnected: false, lastPushUpdate: null, emailListSync: null, @@ -1589,6 +1611,15 @@ export const useEmailStore = create((set, get) => ({ emailHooks.onEmailOpen.emitSync(email); } }, + holdListRow: (rowKey) => { + const { listHold, emails, isScheduledView } = get(); + if (listHold?.rowKey === rowKey) return; + const keywords = new Map(); + for (const email of emails) { + if (listRowKey(email, isScheduledView) === rowKey) keywords.set(email.id, email.keywords); + } + set({ listHold: { rowKey, keywords } }); + }, selectKeyword: (keyword) => set(state => ({ selectedKeyword: keyword, isLoadingMore: false, @@ -1806,8 +1837,8 @@ export const useEmailStore = create((set, get) => ({ // an already-visited account doesn't flash a spinner over the visible mail. const background = opts?.background ?? false; // Loading a real mailbox is a fresh navigation (leaving any cross view), so - // drop the unread/starred retain set. - set(background ? { error: null, retainedInViewIds: new Set() } : { isLoading: true, error: null, retainedInViewIds: new Set() }); // Keep previous emails visible during transition + // drop the unread/starred retain set and the held row. + set(background ? { error: null, retainedInViewIds: new Set(), listHold: null } : { isLoading: true, error: null, retainedInViewIds: new Set(), listHold: null }); // Keep previous emails visible during transition try { const targetMailboxId = mailboxId || get().selectedMailbox; if (targetMailboxId === VIRTUAL_SCHEDULED_MAILBOX_ID) { @@ -4276,15 +4307,17 @@ export const useEmailStore = create((set, get) => ({ // load-more brings them back where they now belong. Not the open // conversation's, though: every action on an email looks it up in this // list, so "mark as unread" on the mail just read must still find it. - // They stay until something else is opened, and load-more does not - // count them. + // Nor the held row, which stays where it was clicked. They stay until + // something else is opened, and load-more does not count them. + const held = get().listHold?.keywords; let retainedInViewIds: Set | undefined; if (keepOpenThread) { const open = get().selectedEmail; const openThread = open ? threadKeyFor(open) : null; retainedInViewIds = new Set(); for (const email of currentEmails) { - if (mergedIds.has(email.id) || threadKeyFor(email) !== openThread) continue; + if (mergedIds.has(email.id)) continue; + if (threadKeyFor(email) !== openThread && !held?.has(email.id)) continue; merged.push(email); mergedIds.add(email.id); retainedInViewIds.add(email.id); @@ -4303,6 +4336,10 @@ export const useEmailStore = create((set, get) => ({ retainedAddedCount = merged.length - beforeLen; } + // The fresh order has the held row where its new read/star/tag state + // sorts; put it back where the list shows it (the order EmailList uses). + merged = placeHeldRows(merged, searchQuery || crossView || hasFilters ? [] : get().listOrder, held); + // Check if anything actually changed to avoid unnecessary re-renders const hasChanged = currentEmails.length !== merged.length || @@ -4424,6 +4461,8 @@ export const useEmailStore = create((set, get) => ({ newExpandedThreadIds.delete(threadKey); } else { newExpandedThreadIds.add(threadKey); + // Expanding marks the thread read; hold it before that lands. + get().holdListRow(threadKey); } set({ expandedThreadIds: newExpandedThreadIds }); @@ -4936,6 +4975,7 @@ export const useEmailStore = create((set, get) => ({ crossView: null, selectedKeyword: null, retainedInViewIds: new Set(), + listHold: null, viewToken: token, }); try { @@ -5020,6 +5060,7 @@ export const useEmailStore = create((set, get) => ({ crossView: view, selectedKeyword: null, retainedInViewIds: new Set(), + listHold: null, viewToken: token, }); try { @@ -5066,6 +5107,7 @@ export const useEmailStore = create((set, get) => ({ crossView: null, unifiedErrors: new Map(), retainedInViewIds: new Set(), + listHold: null, viewToken: state.viewToken + 1, })); }, @@ -5499,6 +5541,15 @@ useEmailStore.subscribe((state, prev) => { useEmailStore.setState(projected); }); +// Opening a message holds its list row (see listHold), however it was opened: +// a click, next/previous, or the pick after a delete. This runs before the +// message is marked read, so the hold records it unread. +useEmailStore.subscribe((state, prev) => { + const email = state.selectedEmail; + if (!email || email.id === prev.selectedEmail?.id) return; + state.holdListRow(listRowKey(email, state.isScheduledView)); +}); + // --------------------------------------------------------------------------- // Boot snapshot: persist the mailbox list and the first page of the current // list so a returning visit paints rows the moment auth resolves, instead of