mirror of
https://github.com/bulwarkmail/webmail.git
synced 2026-10-31 00:12:39 +00:00
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
This commit is contained in:
@@ -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<string | null>(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/<id>` - and the
|
||||
// legacy `?email=<id>` the push service worker used to emit - once the JMAP
|
||||
// session and the mailbox list are up. Runs at most once per mount: after
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
@@ -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<Record<string, unknown>>) {
|
||||
localStorage.setItem(
|
||||
'account-registry',
|
||||
JSON.stringify({
|
||||
state: { accounts, activeAccountId: accounts[0]?.id ?? null, defaultAccountId: null },
|
||||
version: 0,
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
function entry(id: string, overrides: Record<string, unknown> = {}) {
|
||||
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([]);
|
||||
});
|
||||
});
|
||||
@@ -223,6 +223,24 @@ export const useAccountStore = create<AccountState>()(
|
||||
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
|
||||
));
|
||||
},
|
||||
}
|
||||
)
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user