From 8e3b4e71a16f64cfe69bcdd192bffdbca57cae4a Mon Sep 17 00:00:00 2001 From: Linus Rath Date: Sun, 16 Aug 2026 02:19:04 +0200 Subject: [PATCH] fix: open right conversations in shared mailbox #814 --- components/mail/mail-app.tsx | 67 ++++++++++++++++++---------- lib/__tests__/thread-routing.test.ts | 66 +++++++++++++++++++++++++++ lib/thread-routing.ts | 59 ++++++++++++++++++++++++ stores/email-store.ts | 18 ++++---- 4 files changed, 179 insertions(+), 31 deletions(-) create mode 100644 lib/__tests__/thread-routing.test.ts create mode 100644 lib/thread-routing.ts diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 3455234cc..7502ee9cb 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -35,6 +35,7 @@ import { playNotificationSound } from "@/lib/notification-sound"; import { cn } from "@/lib/utils"; import { localizeMailboxName } from "@/lib/mailbox-label"; import { KEYWORD_PREFIX, KEYWORD_PREFIX_LEGACY, groupEmailsByThread } from "@/lib/thread-utils"; +import { resolveThreadRoute } from "@/lib/thread-routing"; import { ErrorBoundary, SidebarErrorFallback, @@ -366,6 +367,14 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { refreshCurrentMailbox, } = useEmailStore(); + // Mailboxes of the account currently being browsed. Mirrors the store's + // action-mailbox resolution so a lookup by `selectedMailbox` hits the same + // list the email fetch used. + const viewMailboxes = useMemo( + () => (viewingAccountId ? (accountMailboxes[viewingAccountId] ?? mailboxes) : mailboxes), + [viewingAccountId, accountMailboxes, mailboxes], + ); + // Load recent recipients (from the Sent folder) for compose autocomplete. // Runs once when the Sent mailbox is known; the store guards against reloads. useEffect(() => { @@ -1493,14 +1502,20 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { const repliedEmail = emailState.emails.find(e => e.id === originalEmailId); if (repliedEmail?.threadId && emailState.expandedThreadIds.has(repliedEmail.threadId)) { // Route to the email's own account so shared/group threads refresh - // from the right server, not the active one. (#281) - const threadClient = emailState.isUnifiedView && repliedEmail.sourceClientAccountId - ? (useAuthStore.getState().getClientForAccount(repliedEmail.sourceClientAccountId) ?? client) + // from the right server, not the active one. (#281, #814) + const route = resolveThreadRoute({ + isUnifiedView: emailState.isUnifiedView, + ref: repliedEmail, + mailboxes: viewMailboxes, + selectedMailbox: emailState.selectedMailbox, + }); + const threadClient = route.clientAccountId + ? (useAuthStore.getState().getClientForAccount(route.clientAccountId) ?? client) : client; - const accountId = emailState.isUnifiedView && repliedEmail.sourceAccountId - ? repliedEmail.sourceAccountId - : client.getAccountId(); - const fullEmails = await threadClient.getThreadEmails(repliedEmail.threadId, accountId); + const fullEmails = await threadClient.getThreadEmails( + repliedEmail.threadId, + route.accountId ?? client.getAccountId(), + ); if (fullEmails.length > 0) { useEmailStore.setState((state) => { const c = new Map(state.threadEmailsCache); @@ -2858,14 +2873,20 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { const repliedEmail = emailState.emails.find(e => e.id === originalEmailId); if (repliedEmail?.threadId && emailState.expandedThreadIds.has(repliedEmail.threadId)) { // Route to the email's own account so shared/group threads refresh from - // the right server, not the active one. (#281) - const threadClient = emailState.isUnifiedView && repliedEmail.sourceClientAccountId - ? (useAuthStore.getState().getClientForAccount(repliedEmail.sourceClientAccountId) ?? client) + // the right server, not the active one. (#281, #814) + const route = resolveThreadRoute({ + isUnifiedView: emailState.isUnifiedView, + ref: repliedEmail, + mailboxes: viewMailboxes, + selectedMailbox: emailState.selectedMailbox, + }); + const threadClient = route.clientAccountId + ? (useAuthStore.getState().getClientForAccount(route.clientAccountId) ?? client) : client; - const accountId = emailState.isUnifiedView && repliedEmail.sourceAccountId - ? repliedEmail.sourceAccountId - : client.getAccountId(); - const fullEmails = await threadClient.getThreadEmails(repliedEmail.threadId, accountId); + const fullEmails = await threadClient.getThreadEmails( + repliedEmail.threadId, + route.accountId ?? client.getAccountId(), + ); if (fullEmails.length > 0) { useEmailStore.setState((state) => { const c = new Map(state.threadEmailsCache); @@ -3017,17 +3038,17 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { setActiveView("viewer"); try { - // In unified/aggregate views the thread may belong to another (possibly - // shared/group) account. Route the fetch to the login it's reachable - // through (`sourceClientAccountId`) and pass its owning JMAP account - // (`sourceAccountId`) so the thread loads from the right server instead of - // the active one (which doesn't have it → empty/body-less). (#281) + // The thread may belong to another account: in unified/aggregate views via + // the email's own source stamp (#281), and in a directly-browsed shared + // folder via the selected mailbox's owner (#814). Either way the fetch has + // to name that account - thread ids are per-account, so an unscoped fetch + // resolves to an unrelated thread in the active one. const ref = thread.emails?.[0]; - const threadClient = isUnifiedView && ref?.sourceClientAccountId - ? (useAuthStore.getState().getClientForAccount(ref.sourceClientAccountId) ?? client) + const route = resolveThreadRoute({ isUnifiedView, ref, mailboxes: viewMailboxes, selectedMailbox }); + const threadClient = route.clientAccountId + ? (useAuthStore.getState().getClientForAccount(route.clientAccountId) ?? client) : client; - const threadAccountId = isUnifiedView ? ref?.sourceAccountId : undefined; - const emails = await threadClient.getThreadEmails(thread.threadId, threadAccountId); + const emails = await threadClient.getThreadEmails(thread.threadId, route.accountId); // Re-stamp the source reference so conversation actions (reply/move/…) // resolve to the right account; the fetched objects don't carry it. if (isUnifiedView && ref) { diff --git a/lib/__tests__/thread-routing.test.ts b/lib/__tests__/thread-routing.test.ts new file mode 100644 index 000000000..b7ce2cf18 --- /dev/null +++ b/lib/__tests__/thread-routing.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect } from 'vitest'; +import { resolveThreadRoute } from '../thread-routing'; +import { makeEmail, makeMailbox } from './helpers/factories'; + +const ownInbox = makeMailbox({ id: 'inbox', role: 'inbox' }); +const sharedInbox = makeMailbox({ + id: 'shared-acct-b__inbox', + originalId: 'inbox', + role: 'inbox', + isShared: true, + accountId: 'acct-b', +}); +const mailboxes = [ownInbox, sharedInbox]; + +describe('resolveThreadRoute', () => { + it('leaves the fetch unscoped for a folder in the user\'s own account', () => { + expect( + resolveThreadRoute({ isUnifiedView: false, ref: makeEmail(), mailboxes, selectedMailbox: 'inbox' }) + ).toEqual({ accountId: undefined }); + }); + + // #814: thread ids are per-account, so a shared folder's thread id names a + // different conversation in the user's own account. Without the owner the + // fetch silently returns that unrelated thread. + it('scopes the fetch to the owner when a shared folder is browsed directly', () => { + expect( + resolveThreadRoute({ + isUnifiedView: false, + // Directly-browsed shared emails carry no source stamp. + ref: makeEmail({ sourceClientAccountId: undefined, sourceAccountId: undefined }), + mailboxes, + selectedMailbox: 'shared-acct-b__inbox', + }) + ).toEqual({ accountId: 'acct-b' }); + }); + + it('prefers the email\'s own source stamp in aggregate views', () => { + expect( + resolveThreadRoute({ + isUnifiedView: true, + ref: makeEmail({ sourceClientAccountId: 'login-a', sourceAccountId: 'acct-c' }), + mailboxes, + // Virtual in unified views, so it must not drive the routing. + selectedMailbox: '__unified_inbox__', + }) + ).toEqual({ clientAccountId: 'login-a', accountId: 'acct-c' }); + }); + + it('falls back to the selected mailbox when an aggregate email is unstamped', () => { + expect( + resolveThreadRoute({ + isUnifiedView: true, + ref: makeEmail({ sourceClientAccountId: 'login-a' }), + mailboxes, + selectedMailbox: 'shared-acct-b__inbox', + }) + ).toEqual({ accountId: 'acct-b' }); + }); + + it('tolerates a missing ref and an unknown mailbox', () => { + expect(resolveThreadRoute({ isUnifiedView: false, mailboxes, selectedMailbox: null })) + .toEqual({ accountId: undefined }); + expect(resolveThreadRoute({ isUnifiedView: true, ref: null, mailboxes, selectedMailbox: 'gone' })) + .toEqual({ accountId: undefined }); + }); +}); diff --git a/lib/thread-routing.ts b/lib/thread-routing.ts new file mode 100644 index 000000000..426706920 --- /dev/null +++ b/lib/thread-routing.ts @@ -0,0 +1,59 @@ +// Account routing for thread fetches (Thread/get + Email/get). +// +// JMAP thread ids are scoped to an account, so the same id names a *different* +// conversation - or none at all - in any other account. A thread fetch that +// doesn't name the owning account is answered by the active account, which +// happily returns whatever thread carries that id there: an unrelated +// conversation from the user's own mailbox. (#814) + +import type { Email, Mailbox } from '@/lib/jmap/types'; + +export interface ThreadRoute { + /** + * AccountEntry.id of the login the thread is reachable through, or undefined + * when the active client already is that login. + */ + clientAccountId?: string; + /** + * Owning JMAP account to scope the fetch to, or undefined for the client's + * own primary account. + */ + accountId?: string; +} + +/** + * Resolves which login and which JMAP account a thread must be fetched from. + * + * Two sources of truth, in order: + * - Aggregate views (unified / cross-account) stamp every email with its own + * `sourceClientAccountId` + `sourceAccountId`; those are unambiguous across + * personal and shared/group sources, so they win. + * - A shared folder browsed directly carries no such stamp - its emails are + * undecorated and reached through the active client - so the owner has to + * come from the selected mailbox instead. + * + * An empty result means "active client, its own account": the normal + * own-mailbox case, and the same request this code sent before shared folders + * existed. + */ +export function resolveThreadRoute({ + isUnifiedView, + ref, + mailboxes, + selectedMailbox, +}: { + isUnifiedView: boolean; + /** Any email known to belong to the thread, used for its source stamp. */ + ref?: Pick | null; + mailboxes: Mailbox[]; + selectedMailbox?: string | null; +}): ThreadRoute { + if (isUnifiedView && ref?.sourceClientAccountId && ref?.sourceAccountId) { + return { clientAccountId: ref.sourceClientAccountId, accountId: ref.sourceAccountId }; + } + + const mailbox = selectedMailbox + ? mailboxes.find((mb) => mb.id === selectedMailbox) + : undefined; + return { accountId: mailbox?.isShared ? mailbox.accountId : undefined }; +} diff --git a/stores/email-store.ts b/stores/email-store.ts index dc6cb260d..b202e61eb 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -6,6 +6,7 @@ import { useSettingsStore } from "@/stores/settings-store"; import { useCalendarStore } from "@/stores/calendar-store"; import { SearchFilters, DEFAULT_SEARCH_FILTERS, buildJMAPFilter, isFilterEmpty } from "@/lib/jmap/search-utils"; import { emailHooks } from "@/lib/plugin-hooks"; +import { resolveThreadRoute } from "@/lib/thread-routing"; import type { ExternalSearchResult } from "@/lib/plugin-types"; import { fetchUnifiedEmails, fetchUnifiedMailboxCounts, searchUnifiedEmails, advancedSearchUnifiedEmails, fetchCrossViewEmails, searchCrossViewEmails, advancedSearchCrossViewEmails, getCrossUnreadTotal, type UnifiedAccountClient, type UnifiedMailboxCounts } from "@/lib/unified-mailbox"; import { useAuthStore } from "@/stores/auth-store"; @@ -3338,18 +3339,19 @@ export const useEmailStore = create((set, get) => ({ // thread (handles shared/group accounts); otherwise fall back to the // selected-mailbox shared-folder logic. (#281) const threadEmail = get().emails.find(e => e.threadId === threadId); + const route = resolveThreadRoute({ + isUnifiedView: get().isUnifiedView, + ref: threadEmail, + mailboxes, + selectedMailbox, + }); let actionClient = resolveActionClient(client); - let accountId: string | undefined; - if (get().isUnifiedView && threadEmail?.sourceClientAccountId && threadEmail?.sourceAccountId) { - actionClient = useAuthStore.getState().getClientForAccount(threadEmail.sourceClientAccountId) ?? actionClient; - accountId = threadEmail.sourceAccountId; - } else { - const mailbox = mailboxes.find(mb => mb.id === selectedMailbox); - accountId = mailbox?.isShared ? mailbox.accountId : undefined; + if (route.clientAccountId) { + actionClient = useAuthStore.getState().getClientForAccount(route.clientAccountId) ?? actionClient; } // Fetch all emails in the thread - const emails = await actionClient.getThreadEmails(threadId, accountId); + const emails = await actionClient.getThreadEmails(threadId, route.accountId); // Re-stamp the source reference so actions on thread emails resolve to the // right account (the fetched objects don't carry it).