From d64249b46db5b535b0fb33f9d0ef57f62b183ff8 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 27 Aug 2026 06:35:26 -0700 Subject: [PATCH] Open a conversation on its first unread message Selecting a thread put you at the newest message. Anything unread above that sat off the top of the pane with nothing to announce it, and the only way to find out was to scroll up -- by which time the auto-mark-read timer had marked the whole thread read anyway, so scrolling up meant scrolling up to mail already counted as seen (#87). Opening at the bottom is right when there is nothing to catch up on and wrong the moment there is. The pane now opens on the oldest message that was unread when the thread was opened, and falls back to the newest when the thread has already been read. mbunkus's out-of-order case is the one that rules out guessing at a position. A participant whose server could not connect for hours delivers a message long after it was written, and it lands in the middle of a conversation that has already moved past it -- so "second to last", or any other fixed offset from the end, finds nothing. Reading the unread set is the only thing that does. Two cases leave the pane where it is: - 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; scrolling to it would push the subject off for nothing It reads the set captured when the thread was opened rather than live `$seen` state, for the same reason expansion does (#69): the mark-read timer must not change the shape of what you are looking at. That also makes the landing stable, because everything above the first unread message is a collapsed row of fixed height -- nothing up there reflows after the scroll. The mock grows a thread that reproduces it: seven messages with the unread one second, four more behind it. Verified against it. Opening the thread lands the unread message flush against the top of the pane at scrollTop 158; the old scroll to the newest message put it at 445, with 287px of the message -- header, sender and unread bar included -- above the fold. On a thread whose first message is the unread one the pane stays at 0 with the subject in view, where before it would have scrolled 333. Once the thread is read, reopening it goes back to the newest message. --- server/src/mock/index.ts | 19 +++++++++ web/src/lib/__tests__/threadScroll.test.ts | 48 ++++++++++++++++++++++ web/src/lib/threadScroll.ts | 35 ++++++++++++++++ web/src/views/mail/ThreadView.tsx | 9 ++-- 4 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 web/src/lib/__tests__/threadScroll.test.ts create mode 100644 web/src/lib/threadScroll.ts 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]);