diff --git a/server/src/mock/index.ts b/server/src/mock/index.ts index 1ad8454..58ebfeb 100644 --- a/server/src/mock/index.ts +++ b/server/src/mock/index.ts @@ -137,6 +137,25 @@ addEmail({ from: ["Demo User", USER], to: "ada@example.org", subject: "Draft: id addEmail({ from: ["Spammy", "win@lottery.example"], subject: "You have WON!!!", daysAgo: 2, mailbox: "junk", unread: true }); addEmail({ from: ["Finance Team", "finance@example.org"], subject: "Invoice 2201 approved", daysAgo: 1, mailbox: "work-inv", unread: true }); addEmail({ from: ["Finance Team", "finance@example.org"], subject: "Invoice 2202 pending", daysAgo: 2, mailbox: "work-inv", unread: true }); +// A thread whose unread message is not the last one: someone's server queued +// their reply for hours, so it landed after messages that answer it and sits in +// the middle of the conversation. Opening this thread at the newest message +// left that reply above the fold until the mark-read timer swept it (#87). +{ + const subj = "Compiler timings for the release"; + const t = addEmail({ from: ["Grace Hopper", "grace@example.org"], subject: subj, daysAgo: 6, mailbox: "inbox", html: true }); + const tid = t.threadId as string; + const reply = (o: { from: [string, string]; daysAgo: number; mailbox: string; to?: string; unread?: boolean; html?: boolean }) => + addEmail({ ...o, subject: `Re: ${subj}`, threadId: tid, inReplyTo: `${t.id}@mock` }); + reply({ from: ["Alan Turing", "alan@example.org"], daysAgo: 5.5, mailbox: "inbox", unread: true }); + // Long enough after the unread one that the thread scrolls: opening at the + // bottom put four messages between the reader and the mail they had not read. + reply({ from: ["Demo User", USER], to: "grace@example.org", daysAgo: 5, mailbox: "sent", html: true }); + reply({ from: ["Grace Hopper", "grace@example.org"], daysAgo: 4.5, mailbox: "inbox" }); + reply({ from: ["Margaret Hamilton", "margaret@example.org"], daysAgo: 4, mailbox: "inbox", html: true }); + reply({ from: ["Demo User", USER], to: "margaret@example.org", daysAgo: 3.5, mailbox: "sent" }); + reply({ from: ["Grace Hopper", "grace@example.org"], daysAgo: 3, mailbox: "inbox", html: true }); +} // Invitation email { const ics = `BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//mock//EN\r\nMETHOD:REQUEST\r\nBEGIN:VEVENT\r\nUID:inv-1@mock\r\nDTSTAMP:20260820T100000Z\r\nDTSTART:20260825T140000Z\r\nDTEND:20260825T150000Z\r\nSUMMARY:Project kickoff\r\nORGANIZER;CN=Ada Lovelace:mailto:ada@example.org\r\nATTENDEE;CN=Demo User;RSVP=TRUE;PARTSTAT=NEEDS-ACTION:mailto:${USER}\r\nLOCATION:Room 4B\r\nEND:VEVENT\r\nEND:VCALENDAR\r\n`; diff --git a/web/src/lib/__tests__/threadScroll.test.ts b/web/src/lib/__tests__/threadScroll.test.ts new file mode 100644 index 0000000..e550cc5 --- /dev/null +++ b/web/src/lib/__tests__/threadScroll.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { threadScrollTarget } from "@/lib/threadScroll"; + +/** + * Issue #87: a conversation opened on its newest message, so unread mail sat + * above the fold with nothing to announce it but a marker you had to scroll up + * to see — and the auto-mark-read timer marked it read while you were still + * looking at the bottom of the thread. + * + * The case that makes "second to last" the wrong answer is out-of-order + * delivery: a message sent hours ago but queued on the sender's server arrives + * last and sorts early. Messages here are in the order the pane renders them, + * oldest first, which is receivedAt order. + */ + +const thread = (n: number) => Array.from({ length: n }, (_, i) => ({ id: `m${i + 1}` })); +const unread = (...ids: string[]) => new Set(ids); + +describe("where a conversation opens", () => { + it("opens on the oldest unread message", () => { + expect(threadScrollTarget(thread(5), unread("m3", "m4"))).toBe("m3"); + }); + + it("opens on an unread message that arrived late and sorted early", () => { + // The one the issue is about: m2 was delivered after m5, so opening at the + // bottom hides it three messages up. + expect(threadScrollTarget(thread(5), unread("m2"))).toBe("m2"); + }); + + it("opens on the newest message when the thread is all read", () => { + expect(threadScrollTarget(thread(5), unread())).toBe("m5"); + }); +}); + +describe("when it leaves the pane where it is", () => { + it("stays at the top when the first message is the unread one", () => { + // Scrolling to it would push the subject off the top for nothing. + expect(threadScrollTarget(thread(4), unread("m1", "m3"))).toBeNull(); + }); + + it("does not scroll a single message", () => { + expect(threadScrollTarget(thread(1), unread("m1"))).toBeNull(); + }); + + it("does not scroll an empty thread", () => { + expect(threadScrollTarget([], unread())).toBeNull(); + }); +}); diff --git a/web/src/lib/threadScroll.ts b/web/src/lib/threadScroll.ts new file mode 100644 index 0000000..4381c5a --- /dev/null +++ b/web/src/lib/threadScroll.ts @@ -0,0 +1,35 @@ +/** + * Where a conversation opens. + * + * It used to open on the newest message, which is wrong whenever anything in + * the thread is unread: the unread mail sits above the fold, and the only clue + * it exists is the marker on a message you have to scroll up to find. The + * auto-mark-read timer then sweeps the whole thread, so scrolling up late is + * scrolling up to mail that is already marked read (#87). + * + * Order is receivedAt, not arrival, so the first unread is not the second-to- + * last message or any other position you can guess at. A thread where one + * participant's server queued a message for hours delivers it late and sorts it + * early -- exactly the case where opening at the bottom hides the most. + * + * Two answers are "don't move": + * + * - a single message, which is already the whole pane + * - the first unread being the first message, where the top of the pane + * shows it anyway, together with the subject + * + * `unread` is the set captured when the thread was opened rather than live + * `$seen` state, for the same reason expansion uses it: the mark-read timer + * must not change the shape of what you are looking at (#69). + */ +export function threadScrollTarget( + messages: readonly T[], + unread: ReadonlySet, +): string | null { + if (messages.length < 2) return null; + const firstUnread = messages.findIndex((m) => unread.has(m.id)); + if (firstUnread === 0) return null; + if (firstUnread > 0) return messages[firstUnread]!.id; + // Nothing unread: the newest message, which is what you came for. + return messages[messages.length - 1]!.id; +} diff --git a/web/src/views/mail/ThreadView.tsx b/web/src/views/mail/ThreadView.tsx index 4e4d76d..6c45c7e 100644 --- a/web/src/views/mail/ThreadView.tsx +++ b/web/src/views/mail/ThreadView.tsx @@ -10,6 +10,7 @@ import { MenuItem, MenuSep, Popover, useMenu } from "@/ui/popover"; import { Spinner } from "@/ui/misc"; import { client } from "@/jmap/client"; import { LabelPicker } from "./LabelPicker"; +import { threadScrollTarget } from "@/lib/threadScroll"; interface Props { threadId: Id; @@ -115,11 +116,13 @@ export function ThreadView({ threadId, mailboxId, onBack, actions, onNavigate, h // eslint-disable-next-line react-hooks/exhaustive-deps }, [messages.map((m) => m.id + (m.keywords.$seen ? "1" : "0")).join(","), settings.markReadDelay]); - // Scroll last expanded into view on load + // Open on the first unread message rather than the newest one (#87). useEffect(() => { if (!messages.length || !scrollRef.current) return; - const el = scrollRef.current.querySelector(`[data-msg-id="${CSS.escape(lastId ?? "")}"]`); - if (el && messages.length > 1) el.scrollIntoView({ block: "start" }); + const target = threadScrollTarget(messages, wasUnread); + if (!target) return; + const el = scrollRef.current.querySelector(`[data-msg-id="${CSS.escape(target)}"]`); + el?.scrollIntoView({ block: "start" }); // eslint-disable-next-line react-hooks/exhaustive-deps }, [threadId, messages.length > 0]);