From a099ab442adc4ba7f5f2f3b891c3e9ee9c8defb3 Mon Sep 17 00:00:00 2001 From: Patrick Rotter Date: Tue, 30 Jun 2026 13:26:56 +0200 Subject: [PATCH] fix: route keyword writes to the email's own account in unified view Tags applied to a shared/group-mailbox message did not persist. Custom keywords (:*), and were written via Email/set against the reaching client's primary account instead of the email's owning account, so the server returned notUpdated without an error and the change was lost on the next reload. toggleStar already threaded an accountId through (#281); the keyword methods did not. Add an optional accountId to updateEmailKeywords and setKeyword and resolve it at the call sites from the email's source account (sourceClientAccountId / sourceAccountId), matching the existing delete/archive routing. Personal sources resolve to the account itself, so behavior there is unchanged. --- app/(main)/[locale]/page.tsx | 59 ++++++++++++----- hooks/use-tag-drop.ts | 18 +++++- lib/__tests__/jmap-keyword-account.test.ts | 74 ++++++++++++++++++++++ lib/jmap/client-interface.ts | 4 +- lib/jmap/client.ts | 8 +-- 5 files changed, 138 insertions(+), 25 deletions(-) create mode 100644 lib/__tests__/jmap-keyword-account.test.ts diff --git a/app/(main)/[locale]/page.tsx b/app/(main)/[locale]/page.tsx index 77af5192..038e45f8 100644 --- a/app/(main)/[locale]/page.tsx +++ b/app/(main)/[locale]/page.tsx @@ -1172,18 +1172,22 @@ export default function Home() { return; } - // Mark the original email with $answered or $forwarded keyword - if (originalEmailId && (effectiveMode === 'reply' || effectiveMode === 'replyAll')) { + // Mark the original email with $answered or $forwarded keyword. Route the + // write to the email's own account so the flag lands on shared/group-mailbox + // messages instead of being dropped against the reaching account. (#281) + if (originalEmailId && (effectiveMode === 'reply' || effectiveMode === 'replyAll' || effectiveMode === 'forward')) { + const s = useEmailStore.getState(); + const orig = s.emails.find(e => e.id === originalEmailId); + const kwClientId = s.isUnifiedView ? orig?.sourceClientAccountId : undefined; + const kwAccountId = s.isUnifiedView ? orig?.sourceAccountId : undefined; + const kwClient = kwClientId + ? (useAuthStore.getState().getClientForAccount(kwClientId) ?? client) + : client; + const keyword = effectiveMode === 'forward' ? '$forwarded' : '$answered'; try { - await client.setKeyword(originalEmailId, '$answered'); + await kwClient.setKeyword(originalEmailId, keyword, kwAccountId); } catch (e) { - debug.error('Failed to set $answered keyword:', e); - } - } else if (originalEmailId && effectiveMode === 'forward') { - try { - await client.setKeyword(originalEmailId, '$forwarded'); - } catch (e) { - debug.error('Failed to set $forwarded keyword:', e); + debug.error(`Failed to set ${keyword} keyword:`, e); } } @@ -1663,8 +1667,20 @@ export default function Home() { } } + // In unified view route the write to the email's own account, reached + // through the login it is reachable via (`sourceClientAccountId`) and + // applied to its owning JMAP account (`sourceAccountId`). For personal + // sources these resolve to the account itself, so behavior is unchanged. + // Without this, tags on shared/group-mailbox messages are written to the + // reaching account and silently dropped by the server. (#281) + const tagClientId = isUnifiedView ? email.sourceClientAccountId : undefined; + const tagAccountId = isUnifiedView ? email.sourceAccountId : undefined; + const tagClient = tagClientId + ? (useAuthStore.getState().getClientForAccount(tagClientId) ?? client) + : client; + // Update email keywords via JMAP - await client.updateEmailKeywords(emailId, keywords); + await tagClient.updateEmailKeywords(emailId, keywords, tagAccountId); // Patch the email in place so the list keeps its scroll/pagination state // instead of being reset to the first page by a full refetch. @@ -2271,11 +2287,22 @@ export default function Home() { return; } - // Mark the original email as answered - try { - await client.setKeyword(originalEmailId, '$answered'); - } catch (e) { - debug.error('Failed to set $answered keyword:', e); + // Mark the original email as answered. Route the write to the email's own + // account so the flag lands on shared/group-mailbox messages instead of + // being dropped against the reaching account. (#281) + { + const s = useEmailStore.getState(); + const orig = s.emails.find(e => e.id === originalEmailId); + const kwClientId = s.isUnifiedView ? orig?.sourceClientAccountId : undefined; + const kwAccountId = s.isUnifiedView ? orig?.sourceAccountId : undefined; + const kwClient = kwClientId + ? (useAuthStore.getState().getClientForAccount(kwClientId) ?? client) + : client; + try { + await kwClient.setKeyword(originalEmailId, '$answered', kwAccountId); + } catch (e) { + debug.error('Failed to set $answered keyword:', e); + } } // Refresh emails to show the sent reply diff --git a/hooks/use-tag-drop.ts b/hooks/use-tag-drop.ts index 61e7a391..2c0d2803 100644 --- a/hooks/use-tag-drop.ts +++ b/hooks/use-tag-drop.ts @@ -74,14 +74,26 @@ export function useTagDrop({ tagId, onSuccess, onError }: UseTagDropOptions): Us for (const emailId of emailIds) { // Read fresh state to avoid stale closures - const currentEmails = useEmailStore.getState().emails; - const email = currentEmails.find(em => em.id === emailId); + const emailState = useEmailStore.getState(); + const email = emailState.emails.find(em => em.id === emailId); const keywords = { ...(email?.keywords || {}) }; // Add the tag without removing existing ones keywords[`$label:${tagId}`] = true; - await client.updateEmailKeywords(emailId, keywords); + // In unified view route the write to the email's own account, reached + // through the login it is reachable via (`sourceClientAccountId`) and + // applied to its owning JMAP account (`sourceAccountId`). For personal + // sources these resolve to the account itself, so behavior is unchanged. + // Without this, tags on shared/group-mailbox messages are written to the + // reaching account and silently dropped by the server. (#281) + const tagClientId = emailState.isUnifiedView ? email?.sourceClientAccountId : undefined; + const tagAccountId = emailState.isUnifiedView ? email?.sourceAccountId : undefined; + const tagClient = tagClientId + ? (useAuthStore.getState().getClientForAccount(tagClientId) ?? client) + : client; + + await tagClient.updateEmailKeywords(emailId, keywords, tagAccountId); } // Refresh the email list diff --git a/lib/__tests__/jmap-keyword-account.test.ts b/lib/__tests__/jmap-keyword-account.test.ts new file mode 100644 index 00000000..7c4585e3 --- /dev/null +++ b/lib/__tests__/jmap-keyword-account.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { JMAPClient } from '../jmap/client'; + +// Regression coverage for the shared-account keyword write path (#281): keyword +// mutations (tags, $answered/$forwarded) on a unified-inbox message must target +// the email's owning account, not the reaching client's primary. Writing to the +// primary account silently no-ops server-side (JMAP returns notUpdated without +// throwing), so the keyword is lost on the next reload. toggleStar already +// threaded accountId through; updateEmailKeywords/setKeyword did not. + +function createClient(): JMAPClient { + const client = new JMAPClient('https://jmap.example.com', 'user@example.com', 'pass'); + Object.assign(client, { + apiUrl: 'https://jmap.example.com/api', + accountId: 'primary-account', + username: 'user@example.com', + }); + return client; +} + +interface JMAPMethodCall { + 0: string; + 1: Record; + 2: string; +} + +function mockEmailSet() { + const captured: JMAPMethodCall[] = []; + const fetchSpy = vi.spyOn(globalThis, 'fetch'); + fetchSpy.mockImplementation(async (_url, init) => { + const body = JSON.parse((init as { body: string }).body) as { methodCalls: JMAPMethodCall[] }; + captured.push(...body.methodCalls); + return new Response(JSON.stringify({ methodResponses: [['Email/set', { updated: {} }, '0']] }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }); + return { captured, fetchSpy }; +} + +describe('JMAP keyword writes route to the email account (#281)', () => { + beforeEach(() => vi.restoreAllMocks()); + afterEach(() => vi.restoreAllMocks()); + + it('updateEmailKeywords sends the explicit accountId', async () => { + const client = createClient(); + const { captured } = mockEmailSet(); + await client.updateEmailKeywords('email-x', { '$label:work': true }, 'shared-account'); + expect(captured[0][0]).toBe('Email/set'); + expect(captured[0][1].accountId).toBe('shared-account'); + }); + + it('updateEmailKeywords falls back to the primary account when none is given', async () => { + const client = createClient(); + const { captured } = mockEmailSet(); + await client.updateEmailKeywords('email-x', { '$label:work': true }); + expect(captured[0][1].accountId).toBe('primary-account'); + }); + + it('setKeyword sends the explicit accountId', async () => { + const client = createClient(); + const { captured } = mockEmailSet(); + await client.setKeyword('email-x', '$answered', 'shared-account'); + expect(captured[0][0]).toBe('Email/set'); + expect(captured[0][1].accountId).toBe('shared-account'); + }); + + it('setKeyword falls back to the primary account when none is given', async () => { + const client = createClient(); + const { captured } = mockEmailSet(); + await client.setKeyword('email-x', '$answered'); + expect(captured[0][1].accountId).toBe('primary-account'); + }); +}); diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index 1cbbe0dc..5b332cfa 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -94,8 +94,8 @@ export interface IJMAPClient { markAsRead(emailId: string, read?: boolean, accountId?: string): Promise; batchMarkAsRead(emailIds: string[], read?: boolean, accountId?: string): Promise; toggleStar(emailId: string, starred: boolean, accountId?: string): Promise; - updateEmailKeywords(emailId: string, keywords: Record): Promise; - setKeyword(emailId: string, keyword: string): Promise; + updateEmailKeywords(emailId: string, keywords: Record, accountId?: string): Promise; + setKeyword(emailId: string, keyword: string, accountId?: string): Promise; migrateKeyword(oldKeyword: string, newKeyword: string): Promise; deleteEmail(emailId: string, accountId?: string): Promise; moveToTrash(emailId: string, trashMailboxId: string, accountId?: string, markAsRead?: boolean): Promise; diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 184c5883..7c04ec45 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -1301,10 +1301,10 @@ export class JMAPClient implements IJMAPClient { ]); } - async updateEmailKeywords(emailId: string, keywords: Record): Promise { + async updateEmailKeywords(emailId: string, keywords: Record, accountId?: string): Promise { await this.request([ ["Email/set", { - accountId: this.accountId, + accountId: accountId || this.accountId, update: { [emailId]: { keywords, @@ -1314,10 +1314,10 @@ export class JMAPClient implements IJMAPClient { ]); } - async setKeyword(emailId: string, keyword: string): Promise { + async setKeyword(emailId: string, keyword: string, accountId?: string): Promise { await this.request([ ["Email/set", { - accountId: this.accountId, + accountId: accountId || this.accountId, update: { [emailId]: { [`keywords/${keyword}`]: true,