From 242fe6aa2d2b99eefca632a7ecafdb0acd7fa678 Mon Sep 17 00:00:00 2001 From: Stefan Hildebrandt <695494+hildebrandttk@users.noreply.github.com> Date: Wed, 2 Sep 2026 09:26:51 +0200 Subject: [PATCH] fix(mail): unified mailbox missed accounts that had not connected yet After a reload the unified mailbox showed only the active account's mail; other accounts appeared only once visited. Intermittent (~1 in 14), and not a timeout - nothing on this path uses setTimeout/AbortSignal/race. checkAuth awaits only the active account and restores the rest in a fire-and-forget background loop, registering each client only after connect() resolves. buildUnifiedAccountClients drops accounts with no live client (`if (!c) continue`). And isConnected was persisted, so it rehydrated stale-true: the account passed the isConnected filter while failing the client lookup, and connectedAccountsSignature already looked complete at mount so nothing re-fired - and the effects keyed on it only recompute counts, never the list. Clear isConnected/hasError on rehydrate so both the filter and the signature describe this session, and refetch the open unified/cross list when an account joins the connected set (growth only, not on first render, not while a search owns the list). Extracted connectedAccountsGrew so the membership check is testable without rendering MailApp. Verified the reset cannot block reconnection: checkAuth reads the full account list and never filters on isConnected. Fixes #950 --- components/mail/mail-app.tsx | 30 ++++++ lib/__tests__/unified-accounts-grew.test.ts | 56 +++++++++++ lib/unified-mailbox.ts | 10 ++ .../__tests__/account-store-rehydrate.test.ts | 97 +++++++++++++++++++ stores/account-store.ts | 18 ++++ 5 files changed, 211 insertions(+) create mode 100644 lib/__tests__/unified-accounts-grew.test.ts create mode 100644 stores/__tests__/account-store-rehydrate.test.ts diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 923d2fbd3..32e44bdaf 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -29,6 +29,7 @@ import { ThreadGroup, Email, Mailbox, isUnifiedMailboxId, UNIFIED_ROLE_BY_ID, CR import { useAccountStore } from "@/stores/account-store"; import { usePolicyStore } from "@/stores/policy-store"; import type { UnifiedAccountClient } from "@/lib/unified-mailbox"; +import { connectedAccountsGrew } from "@/lib/unified-mailbox"; import { KeyboardShortcutsModal } from "@/components/keyboard-shortcuts-modal"; import { useEmailStore, buildUnifiedAccountClients } from "@/stores/email-store"; import { toast } from "@/stores/toast-store"; @@ -1280,6 +1281,35 @@ export function MailApp({ linkSegments }: MailAppProps = {}) { }); }, [enableUnifiedMailbox, includeGroupInUnified, unifiedCrossAccount, activeAccountId, isEmbedded, isAuthenticated, client, mailboxes, connectedAccountsSignature, buildPopulatedUnifiedAccounts, refreshUnifiedCounts, refreshCrossCounts, showCrossUnread, showCrossStarred, showCrossAll]); + // Refetch the open unified/cross list whenever an account joins the connected + // set. Only growth matters - a shrink (sign-out, disconnect) is already + // handled by the flows that cause it, and refetching there would fight them. + const previousConnectedAccountsRef = useRef(null); + useEffect(() => { + const previous = previousConnectedAccountsRef.current; + previousConnectedAccountsRef.current = connectedAccountsSignature; + if (!connectedAccountsGrew(previous, connectedAccountsSignature)) return; + if (!isAuthenticated || !client) return; + + // Read the view at fire time: this effect is keyed on the account set, not + // on the view, so the deps stay free of fast-changing state. + const state = useEmailStore.getState(); + if (!state.isUnifiedView) return; + const { unifiedRole, crossView } = state; + if (!unifiedRole && !crossView) return; + // An active search owns the list; re-running a browse fetch under it would + // replace the user's results with the folder contents. + if (state.searchQuery || !isFilterEmpty(state.searchFilters)) return; + + void buildPopulatedUnifiedAccounts() + .then((populated) => ( + unifiedRole + ? fetchUnifiedEmailsAction(populated, unifiedRole) + : fetchCrossViewAction(populated, crossView!) + )) + .catch(() => { /* per-account failures surface through unifiedErrors */ }); + }, [connectedAccountsSignature, isAuthenticated, client, buildPopulatedUnifiedAccounts, fetchUnifiedEmailsAction, fetchCrossViewAction]); + // Deep links (#733). Applies `/mail/folder|message|thread/` - and the // legacy `?email=` the push service worker used to emit - once the JMAP // session and the mailbox list are up. Runs at most once per mount: after diff --git a/lib/__tests__/unified-accounts-grew.test.ts b/lib/__tests__/unified-accounts-grew.test.ts new file mode 100644 index 000000000..d6117fd95 --- /dev/null +++ b/lib/__tests__/unified-accounts-grew.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from 'vitest'; +import { connectedAccountsGrew } from '../unified-mailbox'; + +/** + * Guards the trigger for the unified list refetch (#950). The unified view can + * be fetched while background accounts are still connecting; when one lands we + * refetch, but only on growth - a shrink is already driven by the flow that + * caused it, and mount must not count or the initial fetch is duplicated. + */ +describe('connectedAccountsGrew (#950)', () => { + it('detects an account joining the set', () => { + expect(connectedAccountsGrew('alice', 'alice,bob')).toBe(true); + }); + + it('detects growth when several accounts land at once', () => { + expect(connectedAccountsGrew('alice', 'alice,bob,carol')).toBe(true); + }); + + it('treats the first render as no growth, so the initial fetch is not duplicated', () => { + expect(connectedAccountsGrew(null, 'alice,bob')).toBe(false); + }); + + it('ignores an unchanged set', () => { + expect(connectedAccountsGrew('alice,bob', 'alice,bob')).toBe(false); + }); + + it('ignores a shrink (sign-out / disconnect owns that flow)', () => { + expect(connectedAccountsGrew('alice,bob', 'alice')).toBe(false); + }); + + it('ignores everything disconnecting', () => { + expect(connectedAccountsGrew('alice,bob', '')).toBe(false); + }); + + // The realistic #950 sequence: reload starts with nothing connected (the + // rehydrate reset), the active account lands, then the background ones. + it('fires for the first account after an empty start', () => { + expect(connectedAccountsGrew('', 'alice')).toBe(true); + }); + + it('fires again as each background account lands', () => { + expect(connectedAccountsGrew('alice', 'alice,bob')).toBe(true); + expect(connectedAccountsGrew('alice,bob', 'alice,bob,carol')).toBe(true); + }); + + // A swap keeps the count identical, so a length comparison would miss it - + // the check must be membership-based. + it('detects a replacement even though the size is unchanged', () => { + expect(connectedAccountsGrew('alice,bob', 'alice,carol')).toBe(true); + }); + + it('does not treat the empty-string baseline as a member', () => { + // ''.split(',') === [''] - a naive Set would contain '' and mis-compare. + expect(connectedAccountsGrew('', '')).toBe(false); + }); +}); diff --git a/lib/unified-mailbox.ts b/lib/unified-mailbox.ts index 598520873..d60bb4745 100644 --- a/lib/unified-mailbox.ts +++ b/lib/unified-mailbox.ts @@ -511,3 +511,13 @@ export function getUnifiedRoles( return roles; } + +/** + * Only growth is interesting. A shrink (sign-out, disconnect) is already driven + * by the flow that caused it, so refetching there would race it. See #950. + */ +export function connectedAccountsGrew(previous: string | null, current: string): boolean { + if (previous === null || previous === current) return false; + const before = new Set(previous ? previous.split(',').filter(Boolean) : []); + return current.split(',').some((id) => id !== '' && !before.has(id)); +} diff --git a/stores/__tests__/account-store-rehydrate.test.ts b/stores/__tests__/account-store-rehydrate.test.ts new file mode 100644 index 000000000..3ecf1085a --- /dev/null +++ b/stores/__tests__/account-store-rehydrate.test.ts @@ -0,0 +1,97 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { useAccountStore } from '../account-store'; + +/** + * `isConnected` / `hasError` describe a live JMAP client, which cannot survive a + * page load - but the whole account entry is persisted, so a fresh tab used to + * rehydrate them as still-connected. The unified mailbox filters on + * `isConnected` and silently drops accounts whose client is not in the map yet, + * and the effects keyed on the connected-accounts signature never re-fired + * because the signature already looked complete at mount. See #950. + */ +function seedStorage(accounts: Array>) { + localStorage.setItem( + 'account-registry', + JSON.stringify({ + state: { accounts, activeAccountId: accounts[0]?.id ?? null, defaultAccountId: null }, + version: 0, + }), + ); +} + +function entry(id: string, overrides: Record = {}) { + return { + id, + cookieSlot: 0, + username: `${id}@example.org`, + serverUrl: 'https://mail.example.org', + displayName: id, + email: `${id}@example.org`, + avatarColor: '#123456', + lastLoginAt: 1_700_000_000_000, + isConnected: true, + hasError: false, + isDefault: false, + ...overrides, + }; +} + +describe('account-store rehydrate (#950)', () => { + beforeEach(() => { + localStorage.clear(); + }); + + it('clears persisted isConnected so it reflects this session, not the last one', async () => { + seedStorage([entry('alice'), entry('bob')]); + + await useAccountStore.persist.rehydrate(); + + const { accounts } = useAccountStore.getState(); + expect(accounts).toHaveLength(2); + // Neither client has connected in this session yet. + expect(accounts.every((a) => a.isConnected === false)).toBe(true); + }); + + it('clears a persisted error state so a stale failure does not stick', async () => { + seedStorage([ + entry('alice', { isConnected: false, hasError: true, errorMessage: 'Temporarily rate limited by server' }), + ]); + + await useAccountStore.persist.rehydrate(); + + const alice = useAccountStore.getState().accounts[0]; + expect(alice.hasError).toBe(false); + expect(alice.errorMessage).toBeUndefined(); + }); + + it('preserves every other field of the entry', async () => { + seedStorage([entry('alice', { displayName: 'Alice A', cookieSlot: 3, isDefault: true })]); + + await useAccountStore.persist.rehydrate(); + + const alice = useAccountStore.getState().accounts[0]; + expect(alice.id).toBe('alice'); + expect(alice.displayName).toBe('Alice A'); + expect(alice.cookieSlot).toBe(3); + expect(alice.isDefault).toBe(true); + expect(alice.email).toBe('alice@example.org'); + expect(alice.serverUrl).toBe('https://mail.example.org'); + expect(alice.lastLoginAt).toBe(1_700_000_000_000); + }); + + it('leaves the active/default account selection alone', async () => { + seedStorage([entry('alice'), entry('bob')]); + + await useAccountStore.persist.rehydrate(); + + expect(useAccountStore.getState().activeAccountId).toBe('alice'); + }); + + it('is a no-op for an empty registry', async () => { + seedStorage([]); + + await useAccountStore.persist.rehydrate(); + + expect(useAccountStore.getState().accounts).toEqual([]); + }); +}); diff --git a/stores/account-store.ts b/stores/account-store.ts index 49835fe42..fa5104a62 100644 --- a/stores/account-store.ts +++ b/stores/account-store.ts @@ -223,6 +223,24 @@ export const useAccountStore = create()( activeAccountId: state.activeAccountId, defaultAccountId: state.defaultAccountId, }), + // `isConnected` / `hasError` describe a *live* JMAP client, which never + // survives a reload - yet the whole entry is persisted, so a fresh tab + // rehydrated them as still-connected. Consumers then treated an account + // whose client had not been restored yet as ready: the unified mailbox + // filters on `isConnected` and silently dropped such accounts when their + // client was missing from the map, and the effects keyed on the + // "connected accounts" signature never re-fired because the signature was + // already complete at mount. Clearing them here makes both reflect this + // session, so the signature genuinely changes as each account connects + // (#950). + onRehydrateStorage: () => (state) => { + if (!state) return; + state.accounts = state.accounts.map((a) => ( + a.isConnected || a.hasError + ? { ...a, isConnected: false, hasError: false, errorMessage: undefined } + : a + )); + }, } ) );