From fe8fff338e8fe51641e0bc9b887bad189cd8b8bb Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 4 Nov 2026 07:02:23 +0100 Subject: [PATCH] fix: let j and k go on from the last opened message after Escape --- .../mail-app-conversation-pane.test.tsx | 44 +++++++++++++++++++ components/mail/mail-app.tsx | 31 ++++++++----- lib/__tests__/list-step.test.ts | 34 ++++++++++++++ lib/list-step.ts | 39 ++++++++++++++++ 4 files changed, 138 insertions(+), 10 deletions(-) create mode 100644 lib/__tests__/list-step.test.ts create mode 100644 lib/list-step.ts diff --git a/components/mail/__tests__/mail-app-conversation-pane.test.tsx b/components/mail/__tests__/mail-app-conversation-pane.test.tsx index a7a54579a..e26eeca3e 100644 --- a/components/mail/__tests__/mail-app-conversation-pane.test.tsx +++ b/components/mail/__tests__/mail-app-conversation-pane.test.tsx @@ -134,6 +134,7 @@ afterEach(() => { vi.unstubAllGlobals(); }); +const selectedId = () => useEmailStore.getState().selectedEmail?.id ?? null; const press = (key: string) => act(() => { fireEvent.keyDown(window, { key }); }); describe('The reading pane in the default layout', () => { @@ -172,3 +173,46 @@ describe('The reading pane in the default layout', () => { expect(useEmailStore.getState().expandedThreadIds.size).toBe(0); }); }); + +describe('j and k after Escape', () => { + beforeEach(() => { + useSettingsStore.setState({ interfaceLayout: 'default', mailLayout: 'split' } as never); + useEmailStore.setState({ listRows: null } as never); + }); + + it('j opens the message after the one that was open, not the first', async () => { + render(); + await screen.findByTestId('reader', {}, { timeout: 5000 }); + await press('j'); + await waitFor(() => expect(selectedId()).toBe('m2')); + await press('Escape'); + await waitFor(() => expect(selectedId()).toBeNull()); + await press('j'); + await waitFor(() => expect(selectedId()).toBe('m3')); + }); + + it('k opens the message before it', async () => { + useEmailStore.setState({ selectedEmail: inbox()[2] } as never); + render(); + await screen.findByTestId('reader', {}, { timeout: 5000 }); + await press('Escape'); + await waitFor(() => expect(selectedId()).toBeNull()); + await press('k'); + await waitFor(() => expect(selectedId()).toBe('m2')); + }); + + it('steps by conversation, in the order of the list', async () => { + useEmailStore.setState({ + listRows: [{ key: 'm1', members: ['m1', 'm3'] }, { key: 'm2', members: ['m2'] }], + } as never); + render(); + await screen.findByTestId('reader', {}, { timeout: 5000 }); + await press('j'); + await waitFor(() => expect(selectedId()).toBe('m2')); + await press('j'); + await act(async () => { await new Promise((resolve) => setTimeout(resolve, 50)); }); + expect(selectedId()).toBe('m2'); + await press('k'); + await waitFor(() => expect(selectedId()).toBe('m1')); + }); +}); diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index ca2b9aae7..d3275629a 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -80,6 +80,7 @@ import { useMailStatusAnnouncements } from "@/hooks/use-mail-status-announcement import { cn, formatDateTime, getMailboxPath } from "@/lib/utils"; import { localizeMailboxName } from "@/lib/mailbox-label"; import { KEYWORD_PREFIX, KEYWORD_PREFIX_LEGACY, emailKeyFor, groupEmailsByThread, threadKeyFor } from "@/lib/thread-utils"; +import { stepListEmail } from "@/lib/list-step"; import { resolveThreadRoute } from "@/lib/thread-routing"; import { ErrorBoundary, @@ -978,23 +979,33 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { ); }; + // The open message's list key, kept after the reader closes. + const lastOpenedKeyRef = useRef(null); + useEffect(() => { + if (selectedEmail) lastOpenedKeyRef.current = emailKeyFor(selectedEmail); + }, [selectedEmail]); // Keyboard shortcuts handlers + // j / k go on from the open message, or from the last one opened once the + // reader is closed (Escape), not from the top. With the conversation in the + // reading pane they step by conversation, as the list's rows do. + const stepFrom = (direction: 1 | -1) => stepListEmail( + activeEmails, + selectedEmail ? emailKeyFor(selectedEmail) : lastOpenedKeyRef.current, + direction, + paneConversationView ? useEmailStore.getState().listRows : null, + ); const keyboardHandlers = useMemo(() => ({ onNextEmail: () => { - if (activeEmails.length === 0) return; - const currentIndex = selectedEmail ? activeEmails.findIndex(e => emailKeyFor(e) === emailKeyFor(selectedEmail)) : -1; - const nextIndex = currentIndex < activeEmails.length - 1 ? currentIndex + 1 : currentIndex; - if (nextIndex >= 0 && nextIndex < activeEmails.length) { - void handleEmailSelect(activeEmails[nextIndex]); + const next = stepFrom(1); + if (next) { + void handleEmailSelect(next); if (mvShell) setCursorOnly(true); } }, onPreviousEmail: () => { - if (activeEmails.length === 0) return; - const currentIndex = selectedEmail ? activeEmails.findIndex(e => emailKeyFor(e) === emailKeyFor(selectedEmail)) : activeEmails.length; - const prevIndex = currentIndex > 0 ? currentIndex - 1 : 0; - if (prevIndex >= 0 && prevIndex < activeEmails.length) { - void handleEmailSelect(activeEmails[prevIndex]); + const previous = stepFrom(-1); + if (previous) { + void handleEmailSelect(previous); if (mvShell) setCursorOnly(true); } }, diff --git a/lib/__tests__/list-step.test.ts b/lib/__tests__/list-step.test.ts new file mode 100644 index 000000000..3445df2b7 --- /dev/null +++ b/lib/__tests__/list-step.test.ts @@ -0,0 +1,34 @@ +import { describe, it, expect } from 'vitest'; +import type { Email } from '@/lib/jmap/types'; +import { stepListEmail } from '../list-step'; + +const mail = (id: string) => ({ id, threadId: `t-${id}` }) as Email; +const emails = [mail('a'), mail('b'), mail('c'), mail('d')]; +const ids = (e: Email | null) => e?.id ?? null; + +describe('stepListEmail', () => { + it('goes on from the given message', () => { + expect(ids(stepListEmail(emails, 'c', 1))).toBe('d'); + expect(ids(stepListEmail(emails, 'c', -1))).toBe('b'); + }); + it('stays put at either end', () => { + expect(ids(stepListEmail(emails, 'd', 1))).toBe('d'); + expect(ids(stepListEmail(emails, 'a', -1))).toBe('a'); + }); + it('starts at the top for j and the bottom for k without a place', () => { + expect(ids(stepListEmail(emails, null, 1))).toBe('a'); + expect(ids(stepListEmail(emails, null, -1))).toBe('d'); + expect(ids(stepListEmail(emails, 'gone', 1))).toBe('a'); + }); + it('counts a conversation once and opens it at its row', () => { + // b's row stands for b and a (its older message); then d, then c. + const rows = [{ key: 'b', members: ['b', 'a'] }, { key: 'd', members: ['d'] }, { key: 'c', members: ['c'] }]; + expect(ids(stepListEmail(emails, 'a', 1, rows))).toBe('d'); + expect(ids(stepListEmail(emails, 'd', 1, rows))).toBe('c'); + expect(ids(stepListEmail(emails, 'd', -1, rows))).toBe('b'); + expect(ids(stepListEmail(emails, null, -1, rows))).toBe('c'); + }); + it('is empty for an empty list', () => { + expect(stepListEmail([], 'a', 1)).toBeNull(); + }); +}); diff --git a/lib/list-step.ts b/lib/list-step.ts new file mode 100644 index 000000000..738b5194c --- /dev/null +++ b/lib/list-step.ts @@ -0,0 +1,39 @@ +import type { Email } from '@/lib/jmap/types'; +import { emailKeyFor } from '@/lib/thread-utils'; + +/** One row of the message list: its own key and the keys it stands for. */ +export interface ListStepRow { + key: string; + members: string[]; +} + +/** + * The message j / k open: the one after (`direction` 1) or before (-1) the + * message `fromKey` names, staying put at either end. `fromKey` is the open + * message, or the last one opened when the reader was closed, so j after + * Escape goes on from there. Without a place in the list, j starts at the + * top and k at the bottom. + * + * With `rows` (the list's rows in screen order) a conversation counts once, + * and opens at its newest message, as a click on its row does: the reading + * pane shows the rest of it. + */ +export function stepListEmail( + emails: readonly Email[], + fromKey: string | null | undefined, + direction: 1 | -1, + rows?: readonly ListStepRow[] | null, +): Email | null { + const byKey = new Map(emails.map((e) => [emailKeyFor(e), e] as const)); + const order: readonly ListStepRow[] = rows && rows.length > 0 + ? rows.filter((row) => byKey.has(row.key)) + : emails.map((e) => ({ key: emailKeyFor(e), members: [] })); + if (order.length === 0) return null; + const at = fromKey + ? order.findIndex((row) => row.key === fromKey || row.members.includes(fromKey)) + : -1; + const target = at < 0 + ? (direction === 1 ? 0 : order.length - 1) + : Math.min(order.length - 1, Math.max(0, at + direction)); + return byKey.get(order[target].key) ?? null; +}