From 7a3a262c87c0b65afce518f5d8b91c96e76de9e3 Mon Sep 17 00:00:00 2001 From: Stefan Hildebrandt <695494+hildebrandttk@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:48:46 +0200 Subject: [PATCH] fix(email): keep just-read/unstarred mail in the open unread/starred view In the unread ('unread') and starred ('starred') cross views, marking a mail read or removing its star flips the very keyword the view filters on (notKeyword:$seen / hasKeyword:$flagged). The push-triggered refresh re-runs that filtered query, so the mail was absent from the fresh page and the merge dropped it - the row vanished from under the cursor on the user's own action. Keep such rows in place: only their status + the counters update (both already applied by markAsRead/toggleStar/batchMarkAsRead), they stay visible until the view is re-opened. A new retainedInViewIds set records the ids the user just read/unstarred while in these views; refreshCurrentMailbox re-inserts those rows at their prior position via mergeRetainedRows. The set is scoped to the two keyword cross views, cleared on navigation (fetchEmails/fetchUnifiedEmails/ fetchCrossView/exitUnifiedView), and dropped per-id when the action is reversed (mark unread / re-star). totalEmails counts the retained rows so hasMore stays correct. Genuinely removed rows (deleted/moved -> absent from the local list) are not resurrected, and a change the user did NOT make in this view (e.g. read on another device, retain set empty) still drops as before. Tests: stores/__tests__/email-store-retained-in-view.test.ts - read keeps the row (unread view), unstar keeps the row (starred view), reversing/ navigation clear retention, external read still drops, 'all' view does not retain. The row-keeping cases fail without the fix. --- .../email-store-retained-in-view.test.ts | 204 ++++++++++++++++++ stores/email-store.ts | 132 +++++++++++- 2 files changed, 331 insertions(+), 5 deletions(-) create mode 100644 stores/__tests__/email-store-retained-in-view.test.ts diff --git a/stores/__tests__/email-store-retained-in-view.test.ts b/stores/__tests__/email-store-retained-in-view.test.ts new file mode 100644 index 000000000..43e2df61f --- /dev/null +++ b/stores/__tests__/email-store-retained-in-view.test.ts @@ -0,0 +1,204 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +/** + * When an unread or starred cross view is open, reading a mail (unread view) or + * removing its star (starred view) must NOT drop it out of the list on the next + * push refresh - the row stays put with its status + counters updated, until the + * view is re-opened. The row only drops if it genuinely left (deleted/moved) or + * left the filter due to a change the user did NOT make in this view (e.g. read + * on another device). + */ + +const { buildMock, fetchCrossViewMock } = vi.hoisted(() => ({ + buildMock: vi.fn(async () => [] as unknown[]), + fetchCrossViewMock: vi.fn(), +})); + +vi.mock('@/lib/unified-mailbox', async (importActual) => { + const actual = await importActual(); + return { + ...actual, + buildUnifiedAccountClients: buildMock, + fetchCrossViewEmails: fetchCrossViewMock, + }; +}); + +import { useEmailStore } from '../email-store'; +import { useAuthStore } from '../auth-store'; +import { useSettingsStore } from '../settings-store'; +import { CROSS_UNREAD, CROSS_STARRED } from '@/lib/jmap/types'; +import type { Email, Mailbox } from '@/lib/jmap/types'; +import type { IJMAPClient } from '@/lib/jmap/client-interface'; + +function makeMailbox(): Mailbox { + return { + id: 'inbox', name: 'Inbox', sortOrder: 0, role: 'inbox', + totalEmails: 0, unreadEmails: 5, totalThreads: 0, unreadThreads: 5, + isSubscribed: true, isShared: false, + } as unknown as Mailbox; +} + +const makeEmail = (id: string, keywords: Record): Email => + ({ + id, + threadId: `t-${id}`, + subject: `mail ${id}`, + receivedAt: '2026-08-07T10:00:00Z', + keywords, + mailboxIds: { inbox: true }, + accountId: 'account-a', + sourceClientAccountId: 'account-a', + sourceAccountId: 'account-a', + }) as unknown as Email; + +function makeClient() { + return { + markAsRead: vi.fn().mockResolvedValue(undefined), + toggleStar: vi.fn().mockResolvedValue(undefined), + } as unknown as IJMAPClient; +} + +// A single-account client whose getEmails would return an empty page for the +// virtual cross id - it must never be consulted for a cross view. +const client = makeClient(); + +function seedView(id: typeof CROSS_UNREAD | typeof CROSS_STARRED, view: 'unread' | 'starred', emails: Email[]) { + useSettingsStore.setState({ emailsPerPage: 25 }); + useAuthStore.setState({ + activeAccountId: 'account-a', + getClientForAccount: (a: string) => (a === 'account-a' ? client : undefined) as never, + } as never); + useEmailStore.setState({ + isUnifiedView: true, + unifiedRole: null, + crossView: view, + selectedMailbox: id, + selectedKeyword: null, + searchQuery: '', + mailboxes: [makeMailbox()], + accountMailboxes: { 'account-a': [makeMailbox()] }, + processingReadStatus: new Set(), + selectedEmailIds: new Set(), + retainedInViewIds: new Set(), + emails, + totalEmails: emails.length, + }); +} + +describe('retained-in-view: unread cross view', () => { + beforeEach(() => { + vi.clearAllMocks(); + buildMock.mockResolvedValue([]); + }); + + it('keeps a mail the user just read; the fresh unread page no longer lists it', async () => { + const a = makeEmail('a', {}); + const b = makeEmail('b', {}); + seedView(CROSS_UNREAD, 'unread', [a, b]); + + await useEmailStore.getState().markAsRead(client, 'a', true); + expect(useEmailStore.getState().retainedInViewIds.has('a')).toBe(true); + + // Push refresh: the unread filter drops 'a' (now $seen); only 'b' comes back. + fetchCrossViewMock.mockResolvedValue({ emails: [makeEmail('b', {})], hasMore: false, total: 1, errors: new Map() }); + await useEmailStore.getState().refreshCurrentMailbox(client); + + const emails = useEmailStore.getState().emails; + expect(emails.map(e => e.id)).toEqual(['a', 'b']); // 'a' retained in place + expect(emails.find(e => e.id === 'a')?.keywords?.$seen).toBe(true); + // Counter reflects the two visible rows, not just the server's unread total. + expect(useEmailStore.getState().totalEmails).toBe(2); + }); + + it('does NOT retain a mail read elsewhere (retain set empty): it drops on refresh', async () => { + const a = makeEmail('a', {}); + const b = makeEmail('b', {}); + seedView(CROSS_UNREAD, 'unread', [a, b]); + + // No local action -> retain set stays empty. 'a' left the unread filter + // (e.g. read on another device); the refresh should drop it. + fetchCrossViewMock.mockResolvedValue({ emails: [makeEmail('b', {})], hasMore: false, total: 1, errors: new Map() }); + await useEmailStore.getState().refreshCurrentMailbox(client); + + expect(useEmailStore.getState().emails.map(e => e.id)).toEqual(['b']); + }); + + it('marking the mail unread again drops the retention', async () => { + const a = makeEmail('a', { $seen: true }); + seedView(CROSS_UNREAD, 'unread', [a]); + useEmailStore.setState({ retainedInViewIds: new Set(['a']) }); + + await useEmailStore.getState().markAsRead(client, 'a', false); + expect(useEmailStore.getState().retainedInViewIds.has('a')).toBe(false); + }); + + it('clears retention when navigating away to a real mailbox', async () => { + const a = makeEmail('a', {}); + seedView(CROSS_UNREAD, 'unread', [a]); + await useEmailStore.getState().markAsRead(client, 'a', true); + expect(useEmailStore.getState().retainedInViewIds.size).toBe(1); + + // Navigating to a real folder is a fresh load; retention must reset. + await useEmailStore.getState().fetchEmails(client, 'inbox').catch(() => {}); + expect(useEmailStore.getState().retainedInViewIds.size).toBe(0); + }); +}); + +describe('retained-in-view: starred cross view', () => { + beforeEach(() => { + vi.clearAllMocks(); + buildMock.mockResolvedValue([]); + }); + + it('keeps a mail the user just un-starred; the fresh starred page omits it', async () => { + const a = makeEmail('a', { $flagged: true }); + const b = makeEmail('b', { $flagged: true }); + seedView(CROSS_STARRED, 'starred', [a, b]); + + await useEmailStore.getState().toggleStar(client, 'a'); // removes the star + expect(useEmailStore.getState().retainedInViewIds.has('a')).toBe(true); + + fetchCrossViewMock.mockResolvedValue({ emails: [makeEmail('b', { $flagged: true })], hasMore: false, total: 1, errors: new Map() }); + await useEmailStore.getState().refreshCurrentMailbox(client); + + const emails = useEmailStore.getState().emails; + expect(emails.map(e => e.id)).toEqual(['a', 'b']); + expect(emails.find(e => e.id === 'a')?.keywords?.$flagged).toBe(false); + }); + + it('re-starring drops the retention', async () => { + const a = makeEmail('a', {}); // currently unflagged + seedView(CROSS_STARRED, 'starred', [a]); + useEmailStore.setState({ retainedInViewIds: new Set(['a']) }); + + await useEmailStore.getState().toggleStar(client, 'a'); // adds the star back + expect(useEmailStore.getState().retainedInViewIds.has('a')).toBe(false); + }); +}); + +describe('retained-in-view: scope', () => { + beforeEach(() => { + vi.clearAllMocks(); + buildMock.mockResolvedValue([]); + }); + + it('does not retain on read in the "all" cross view (no keyword filter to fall out of)', async () => { + const a = makeEmail('a', {}); + // 'all' view: reading never removes the row, so nothing to retain. + useSettingsStore.setState({ emailsPerPage: 25 }); + useAuthStore.setState({ + activeAccountId: 'account-a', + getClientForAccount: (acc: string) => (acc === 'account-a' ? client : undefined) as never, + } as never); + useEmailStore.setState({ + isUnifiedView: true, unifiedRole: null, crossView: 'all', selectedMailbox: '__cross_all__', + selectedKeyword: null, searchQuery: '', mailboxes: [makeMailbox()], + accountMailboxes: { 'account-a': [makeMailbox()] }, + processingReadStatus: new Set(), selectedEmailIds: new Set(), retainedInViewIds: new Set(), + emails: [a], totalEmails: 1, + }); + + await useEmailStore.getState().markAsRead(client, 'a', true); + expect(useEmailStore.getState().retainedInViewIds.size).toBe(0); + }); +}); diff --git a/stores/email-store.ts b/stores/email-store.ts index dfdaaa04a..edde9287e 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -60,6 +60,10 @@ interface EmailStore { quota: { used: number; total: number } | null; processingReadStatus: Set; // Track emails being marked as read/unread selectedEmailIds: Set; // Track selected emails for batch operations + // Emails the user just read (unread view) or unstarred (starred view) that + // should stay visible in that self-filtering cross view until it is re-opened, + // instead of vanishing on the next push refresh. Cleared on navigation. + retainedInViewIds: Set; hasMoreEmails: boolean; // Track if more emails are available to load totalEmails: number; // Total number of emails in the current mailbox/query isPushConnected: boolean; // Track if push notifications are connected @@ -804,6 +808,72 @@ if (typeof window !== 'undefined') { }); } +/** + * The two cross views whose filter is the very keyword the read/star actions + * flip: 'unread' (notKeyword $seen) and 'starred' (hasKeyword $flagged). In + * those views a just-read / just-unstarred mail no longer matches the query, so + * a naive push refresh drops it. We keep such rows (see retainedInViewIds). + * The 'all' view has no keyword filter and needs no retention. + */ +function retainingCrossView(state: { isUnifiedView: boolean; crossView: CrossView | null }): 'unread' | 'starred' | null { + if (!state.isUnifiedView) return null; + return state.crossView === 'unread' || state.crossView === 'starred' ? state.crossView : null; +} + +/** + * Immutably add/remove an id from the retain set. Returns the same set instance + * when nothing changes (op 'none', or a no-op add/remove) to avoid needless + * re-renders. + */ +function updateRetainedForView(current: Set, op: 'add' | 'remove' | 'none', id: string): Set { + if (op === 'none') return current; + if (op === 'add') { + if (current.has(id)) return current; + return new Set(current).add(id); + } + if (!current.has(id)) return current; + const next = new Set(current); + next.delete(id); + return next; +} + +/** + * Re-insert rows the user just read/unstarred in an unread/starred cross view + * that fell out of the fresh keyword-filtered page, keeping them at their prior + * position so the row updates its status + counter instead of vanishing from + * under the cursor. Rows genuinely gone (deleted/moved, hence absent from + * `previous`) are NOT resurrected - only ids still present locally are kept. + * Returns the original `merged` untouched when nothing needs retaining. + */ +function mergeRetainedRows(previous: Email[], merged: Email[], retainedIds: Set): Email[] { + if (retainedIds.size === 0) return merged; + const mergedById = new Map(merged.map(e => [e.id, e] as const)); + const toRetain = previous.filter(e => retainedIds.has(e.id) && !mergedById.has(e.id)); + if (toRetain.length === 0) return merged; + + const previousIds = new Set(previous.map(e => e.id)); + const out: Email[] = []; + const emitted = new Set(); + // Genuinely new arrivals (not previously loaded) keep their fresh top slot. + for (const e of merged) { + if (!previousIds.has(e.id)) { out.push(e); emitted.add(e.id); } + } + // Then previous order: fresh copy if the row still matches, retained local + // copy otherwise. Rows absent from both `merged` and the retain set are the + // truly-removed ones and get dropped here. + for (const e of previous) { + if (emitted.has(e.id)) continue; + const fresh = mergedById.get(e.id); + if (fresh) { out.push(fresh); emitted.add(e.id); } + else if (retainedIds.has(e.id)) { out.push(e); emitted.add(e.id); } + } + // Any deeper-page rows merged in but not covered above, in merged order. + for (const e of merged) { + if (!emitted.has(e.id)) { out.push(e); emitted.add(e.id); } + } + return out; +} + export const useEmailStore = create((set, get) => ({ emails: [], mailboxes: [], @@ -819,6 +889,7 @@ export const useEmailStore = create((set, get) => ({ quota: null, processingReadStatus: new Set(), selectedEmailIds: new Set(), + retainedInViewIds: new Set(), lastSelectedEmailId: null, hasMoreEmails: false, totalEmails: 0, @@ -1074,7 +1145,9 @@ export const useEmailStore = create((set, get) => ({ // repopulates the list without showing the loading overlay, so switching to // an already-visited account doesn't flash a spinner over the visible mail. const background = opts?.background ?? false; - set(background ? { error: null } : { isLoading: true, error: null }); // Keep previous emails visible during transition + // Loading a real mailbox is a fresh navigation (leaving any cross view), so + // drop the unread/starred retain set. + set(background ? { error: null, retainedInViewIds: new Set() } : { isLoading: true, error: null, retainedInViewIds: new Set() }); // Keep previous emails visible during transition try { const targetMailboxId = mailboxId || get().selectedMailbox; if (targetMailboxId === VIRTUAL_SCHEDULED_MAILBOX_ID) { @@ -1592,6 +1665,16 @@ export const useEmailStore = create((set, get) => ({ : mailbox, ); + // In the unread cross view, a mail just marked read no longer matches the + // filter. Retain it so the row stays put (status + counter update only) + // rather than vanishing on the next push refresh; marking it unread again + // makes it match, so drop the retention. + const retainedInViewIds = updateRetainedForView( + state.retainedInViewIds, + retainingCrossView(state) === 'unread' ? (read ? 'add' : 'remove') : 'none', + emailId, + ); + return { emails: state.emails.map(e => e.id === emailId ? { ...e, keywords: { ...e.keywords, $seen: read } } : e @@ -1599,6 +1682,7 @@ export const useEmailStore = create((set, get) => ({ selectedEmail: state.selectedEmail?.id === emailId ? { ...state.selectedEmail, keywords: { ...state.selectedEmail.keywords, $seen: read } } : state.selectedEmail, + retainedInViewIds, ...mailboxPatch, // Same delta, applied to every tag this email carries, so the sidebar // tag badges track the folder counters instead of going stale. @@ -2212,7 +2296,15 @@ export const useEmailStore = create((set, get) => ({ ), selectedEmail: state.selectedEmail?.id === emailId ? { ...state.selectedEmail, keywords: { ...state.selectedEmail.keywords, $flagged: !isFlagged } } - : state.selectedEmail + : state.selectedEmail, + // In the starred cross view, un-starring drops the mail out of the + // hasKeyword:$flagged filter. Retain the row so it stays visible until the + // view is re-opened; re-starring makes it match again, so drop retention. + retainedInViewIds: updateRetainedForView( + state.retainedInViewIds, + retainingCrossView(state) === 'starred' ? (isFlagged ? 'add' : 'remove') : 'none', + emailId, + ), })); } catch (error) { set({ @@ -2309,10 +2401,21 @@ export const useEmailStore = create((set, get) => ({ .map(email => ({ keywords: email.keywords, delta: read ? -1 : 1 })), ); + // In the unread cross view, retain every mail we just marked read so the + // rows stay put (status + counter update only); marking unread drops them. + let retainedInViewIds = get().retainedInViewIds; + if (retainingCrossView(get()) === 'unread') { + const op = read ? 'add' : 'remove'; + for (const e of affectedEmails) { + retainedInViewIds = updateRetainedForView(retainedInViewIds, op, e.id); + } + } + set({ emails: updatedEmails, ...mailboxPatch, tagCounts, + retainedInViewIds, selectedEmailIds: new Set(), isLoading: false }); @@ -3016,7 +3119,7 @@ export const useEmailStore = create((set, get) => ({ // append the whole previous list: drafts are saved as destroy+create, so // the old draft can disappear from the refreshed first page and must not // be reintroduced from stale local state. - const merged: Email[] = [...refreshedEmails]; + let merged: Email[] = [...refreshedEmails]; const mergedIds = new Set(refreshedEmails.map((e: Email) => e.id)); const insertedCount = Math.max((result.total || 0) - previousTotal, 0); // Derive the cutoff from the page size, not from the refreshed list's @@ -3035,6 +3138,18 @@ export const useEmailStore = create((set, get) => ({ } } + // Keep rows the user just read/unstarred in the unread/starred cross view: + // the fresh keyword-filtered page dropped them, but they should stay put + // (their status + the counters already updated in the action) until the + // view is re-opened. Only applies to those two views; the retain set is + // empty otherwise. + let retainedAddedCount = 0; + if (retainingCrossView({ isUnifiedView, crossView })) { + const beforeLen = merged.length; + merged = mergeRetainedRows(currentEmails, merged, get().retainedInViewIds); + retainedAddedCount = merged.length - beforeLen; + } + // Check if anything actually changed to avoid unnecessary re-renders const hasChanged = currentEmails.length !== merged.length || @@ -3049,9 +3164,13 @@ export const useEmailStore = create((set, get) => ({ }); if (hasChanged) { + // Retained rows sit beyond the server's keyword-filtered count, so add + // them to the total; otherwise the list would claim fewer items than it + // shows and mis-compute hasMore. + const effectiveTotal = (result.total || 0) + retainedAddedCount; // hasMore should reflect whether there are still more emails beyond // what we have loaded, using the fresh total from the server. - const hasMore = merged.length < (result.total || 0); + const hasMore = merged.length < effectiveTotal; // Invalidate thread email caches for threads whose composition changed // so expanded threads pick up new/removed emails. @@ -3094,7 +3213,7 @@ export const useEmailStore = create((set, get) => ({ return { emails: merged, hasMoreEmails: hasMore, - totalEmails: result.total, + totalEmails: effectiveTotal, threadEmailsCache: newCache, ...(unifiedErrors !== undefined ? { unifiedErrors } : {}), }; @@ -3555,6 +3674,7 @@ export const useEmailStore = create((set, get) => ({ unifiedRole: role, crossView: null, selectedKeyword: null, + retainedInViewIds: new Set(), }); try { const emailsPerPage = useSettingsStore.getState().emailsPerPage; @@ -3630,6 +3750,7 @@ export const useEmailStore = create((set, get) => ({ unifiedRole: null, crossView: view, selectedKeyword: null, + retainedInViewIds: new Set(), }); try { const emailsPerPage = useSettingsStore.getState().emailsPerPage; @@ -3670,6 +3791,7 @@ export const useEmailStore = create((set, get) => ({ unifiedRole: null, crossView: null, unifiedErrors: new Map(), + retainedInViewIds: new Set(), }); },