mirror of
https://github.com/bulwarkmail/webmail.git
synced 2026-10-31 00:12:39 +00:00
fix: report a failed message-list read instead of an empty folder
`getEmails` and `getSomeEmails` caught every failure and answered with an empty result. A refresh during a server hiccup therefore emptied the message list, and a push delta whose follow-up read failed treated every updated row as gone from the folder while still moving the sync state past those changes, so the missed mail only came back on a full reload. Both now throw. A refresh that fails keeps the list on screen, a delta that cannot be read falls back to the full refresh, and the unified views already record per-account failures instead of showing nothing.
This commit is contained in:
Linus Rath
@@ -85,17 +85,24 @@ export function ProFolderTabBody({ tabId, data }: ProFolderTabBodyProps) {
|
||||
if (!client) return;
|
||||
const seq = ++fetchSeqRef.current;
|
||||
if (position === 0) setIsLoading(true); else setIsLoadingMore(true);
|
||||
// getEmails never throws - it reports failures as an empty page.
|
||||
const result = await client.getEmails(jmapMailboxId, jmapAccountId, emailsPerPage, position, undefined, true, undefined, getMessageListOrderFor(mailboxRole));
|
||||
if (seq !== fetchSeqRef.current) return;
|
||||
setEmails((prev) => {
|
||||
if (position === 0) return result.emails;
|
||||
const known = new Set(prev.map((e) => e.id));
|
||||
return [...prev, ...result.emails.filter((e) => !known.has(e.id))];
|
||||
});
|
||||
setTotal(result.total);
|
||||
setHasMore(result.hasMore);
|
||||
if (position === 0) setIsLoading(false); else setIsLoadingMore(false);
|
||||
try {
|
||||
const result = await client.getEmails(jmapMailboxId, jmapAccountId, emailsPerPage, position, undefined, true, undefined, getMessageListOrderFor(mailboxRole));
|
||||
if (seq !== fetchSeqRef.current) return;
|
||||
setEmails((prev) => {
|
||||
if (position === 0) return result.emails;
|
||||
const known = new Set(prev.map((e) => e.id));
|
||||
return [...prev, ...result.emails.filter((e) => !known.has(e.id))];
|
||||
});
|
||||
setTotal(result.total);
|
||||
setHasMore(result.hasMore);
|
||||
} catch (error) {
|
||||
// Keep what is on screen: a failed page is not an empty folder.
|
||||
console.error('Failed to load folder tab:', error);
|
||||
} finally {
|
||||
if (seq === fetchSeqRef.current) {
|
||||
if (position === 0) setIsLoading(false); else setIsLoadingMore(false);
|
||||
}
|
||||
}
|
||||
}, [client, jmapMailboxId, jmapAccountId, emailsPerPage, mailboxRole]);
|
||||
|
||||
useEffect(() => {
|
||||
|
||||
@@ -143,6 +143,26 @@ describe('JMAPClient resilience', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('reading lists when the server fails', () => {
|
||||
// An empty answer read as an empty folder: a failed refresh emptied the
|
||||
// list, and a failed delta read treated every updated row as gone.
|
||||
it('getEmails rejects instead of returning an empty page', async () => {
|
||||
const client = await createConnectedClient();
|
||||
fetchSpy.mockResolvedValueOnce(mockFetchResponse(200, {
|
||||
methodResponses: [['error', { type: 'serverFail', description: 'backend down' }, '0']],
|
||||
}));
|
||||
await expect(client.getEmails('inbox')).rejects.toThrow('backend down');
|
||||
});
|
||||
|
||||
it('getSomeEmails rejects instead of returning nothing', async () => {
|
||||
const client = await createConnectedClient();
|
||||
fetchSpy.mockResolvedValueOnce(mockFetchResponse(200, {
|
||||
methodResponses: [['error', { type: 'serverFail' }, '0']],
|
||||
}));
|
||||
await expect(client.getSomeEmails(['e1'])).rejects.toThrow('serverFail');
|
||||
});
|
||||
});
|
||||
|
||||
describe('isReplaySafeRequest', () => {
|
||||
const batch = (...methods: string[]) => ({
|
||||
method: 'POST',
|
||||
|
||||
+117
-114
@@ -188,6 +188,13 @@ export function isReplaySafeRequest(init?: Parameters<typeof fetch>[1]): boolean
|
||||
}
|
||||
}
|
||||
|
||||
/** The first method error in a JMAP response, for an Error message. */
|
||||
function methodErrorMessage(response: JMAPResponse, fallback: string): string {
|
||||
const failed = response.methodResponses?.find(([name]) => name === 'error');
|
||||
const detail = failed?.[1] as { type?: string; description?: string } | undefined;
|
||||
return detail?.description || detail?.type || fallback;
|
||||
}
|
||||
|
||||
/** A scheduled send later than the server's hold limit. */
|
||||
export class ScheduleTooLateError extends Error {
|
||||
constructor(readonly maxSeconds?: number) {
|
||||
@@ -932,43 +939,41 @@ export class JMAPClient implements IJMAPClient {
|
||||
this.authHeader = `Bearer ${token}`;
|
||||
}
|
||||
|
||||
// Throws when the server does not answer: an empty result would read as
|
||||
// "these messages are gone", and callers would drop them from the list.
|
||||
async getSomeEmails(emailsId: string[], accountId?: string): Promise<Email[]> {
|
||||
try {
|
||||
const targetAccountId = accountId || this.accountId;
|
||||
if (!emailsId || emailsId.length === 0) {
|
||||
return [];
|
||||
}
|
||||
|
||||
const emails: Email[] = [];
|
||||
|
||||
for (const batchIds of batched(emailsId, this.getMaxObjectsInGet())) {
|
||||
const response = await this.request([
|
||||
["Email/get", {
|
||||
accountId: targetAccountId,
|
||||
ids: batchIds,
|
||||
properties: [...EMAIL_LIST_PROPERTIES],
|
||||
}, "0"],
|
||||
]);
|
||||
|
||||
const getResponse = response.methodResponses?.[0]?.[1];
|
||||
if (response.methodResponses?.[0]?.[0] === "Email/get" && getResponse) {
|
||||
emails.push(...((getResponse.list || []) as Email[]));
|
||||
}
|
||||
}
|
||||
|
||||
emails.sort((a: Email, b: Email) =>
|
||||
new Date(b.receivedAt).getTime() - new Date(a.receivedAt).getTime()
|
||||
);
|
||||
|
||||
if (accountId && accountId !== this.accountId) {
|
||||
namespaceMailboxIds(emails, accountId);
|
||||
}
|
||||
|
||||
return emails;
|
||||
} catch (error) {
|
||||
console.error('Failed to get specific emails:', error);
|
||||
const targetAccountId = accountId || this.accountId;
|
||||
if (!emailsId || emailsId.length === 0) {
|
||||
return [];
|
||||
}
|
||||
|
||||
const emails: Email[] = [];
|
||||
|
||||
for (const batchIds of batched(emailsId, this.getMaxObjectsInGet())) {
|
||||
const response = await this.request([
|
||||
["Email/get", {
|
||||
accountId: targetAccountId,
|
||||
ids: batchIds,
|
||||
properties: [...EMAIL_LIST_PROPERTIES],
|
||||
}, "0"],
|
||||
]);
|
||||
|
||||
const getResponse = response.methodResponses?.[0]?.[1];
|
||||
if (response.methodResponses?.[0]?.[0] !== "Email/get" || !getResponse) {
|
||||
throw new Error(methodErrorMessage(response, 'Failed to get emails'));
|
||||
}
|
||||
emails.push(...((getResponse.list || []) as Email[]));
|
||||
}
|
||||
|
||||
emails.sort((a: Email, b: Email) =>
|
||||
new Date(b.receivedAt).getTime() - new Date(a.receivedAt).getTime()
|
||||
);
|
||||
|
||||
if (accountId && accountId !== this.accountId) {
|
||||
namespaceMailboxIds(emails, accountId);
|
||||
}
|
||||
|
||||
return emails;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -1834,88 +1839,86 @@ export class JMAPClient implements IJMAPClient {
|
||||
return { sort: buildEmailSort(order, { pinnedFirst, polarity }), keywordSortSupported: true };
|
||||
}
|
||||
|
||||
// Throws when the page cannot be read. Answering with an empty page made a
|
||||
// failed refresh empty the list, and callers could not tell it from an
|
||||
// empty folder.
|
||||
async getEmails(mailboxId?: string, accountId?: string, limit: number = 50, position: number = 0, hasKeyword?: string, pinnedFirst?: boolean, extraFilter?: Record<string, unknown>, order: SortLevel[] = []): Promise<{ emails: Email[], hasMore: boolean, total: number, state?: string }> {
|
||||
try {
|
||||
const targetAccountId = accountId || this.accountId;
|
||||
const simple: { inMailbox?: string; hasKeyword?: string } = {};
|
||||
if (mailboxId) {
|
||||
simple.inMailbox = mailboxId;
|
||||
}
|
||||
if (hasKeyword) {
|
||||
simple.hasKeyword = hasKeyword;
|
||||
}
|
||||
// `extraFilter` is an arbitrary FilterCondition/FilterOperator ANDed
|
||||
// into the view - the message-list category tabs' search contract.
|
||||
const filter: Record<string, unknown> = extraFilter
|
||||
? {
|
||||
operator: "AND",
|
||||
conditions: [
|
||||
...(Object.keys(simple).length > 0 ? [simple] : []),
|
||||
extraFilter,
|
||||
],
|
||||
}
|
||||
: simple;
|
||||
// Pinned-first and the configured list order (#718) use the hasKeyword
|
||||
// sort comparator (RFC 8621 §4.4.2); every page of a view must use the
|
||||
// same sort or pagination tears, so the sort is built from settings the
|
||||
// same way on the first page, load-more and the push refresh.
|
||||
const built = await this.buildListSort(targetAccountId, pinnedFirst === true, order);
|
||||
const query = (sort: ReturnType<typeof buildEmailSort>) => this.request([
|
||||
["Email/query", {
|
||||
accountId: targetAccountId,
|
||||
filter,
|
||||
sort,
|
||||
limit,
|
||||
position,
|
||||
calculateTotal: true,
|
||||
}, "0"],
|
||||
["Email/get", {
|
||||
accountId: targetAccountId,
|
||||
"#ids": { resultOf: "0", name: "Email/query", path: "/ids" },
|
||||
properties: [...EMAIL_LIST_PROPERTIES],
|
||||
}, "1"],
|
||||
]);
|
||||
|
||||
let response = await query(built.sort);
|
||||
// A server that advertises nothing but refuses hasKeyword fails the whole
|
||||
// query. Remember that and retry once without the keyword comparators
|
||||
// rather than showing an empty folder.
|
||||
if (
|
||||
built.keywordSortSupported &&
|
||||
response.methodResponses?.[0]?.[0] === "error" &&
|
||||
(response.methodResponses[0][1] as { type?: string })?.type === "unsupportedSort"
|
||||
) {
|
||||
this.keywordSortUnsupported.add(targetAccountId);
|
||||
response = await query(buildEmailSort(order, { pinnedFirst, keywordSortSupported: false }));
|
||||
}
|
||||
|
||||
const queryResponse = response.methodResponses?.[0]?.[1];
|
||||
const getResponse = response.methodResponses?.[1]?.[1];
|
||||
|
||||
if (response.methodResponses?.[1]?.[0] === "Email/get" && getResponse) {
|
||||
const emails = (getResponse.list || []) as Email[];
|
||||
// Sort client-side as safety net - some servers may not honour
|
||||
// the query sort for large mailboxes without additional filters.
|
||||
// Must mirror the query sort, or it would undo the configured order.
|
||||
emails.sort(compareEmails(order, { pinnedFirst }));
|
||||
const total = queryResponse?.total || 0;
|
||||
const hasMore = computeHasMore(position, emails.length, total, limit);
|
||||
|
||||
if (accountId && accountId !== this.accountId) {
|
||||
namespaceMailboxIds(emails, accountId);
|
||||
}
|
||||
|
||||
// The Email collection state this page was read at; the store keeps
|
||||
// it so a later push can be applied with Email/changes.
|
||||
const state = typeof getResponse.state === 'string' ? getResponse.state : undefined;
|
||||
return { emails, hasMore, total, state };
|
||||
}
|
||||
|
||||
return { emails: [], hasMore: false, total: 0 };
|
||||
} catch (error) {
|
||||
console.error('Failed to get emails:', error);
|
||||
return { emails: [], hasMore: false, total: 0 };
|
||||
const targetAccountId = accountId || this.accountId;
|
||||
const simple: { inMailbox?: string; hasKeyword?: string } = {};
|
||||
if (mailboxId) {
|
||||
simple.inMailbox = mailboxId;
|
||||
}
|
||||
if (hasKeyword) {
|
||||
simple.hasKeyword = hasKeyword;
|
||||
}
|
||||
// `extraFilter` is an arbitrary FilterCondition/FilterOperator ANDed
|
||||
// into the view - the message-list category tabs' search contract.
|
||||
const filter: Record<string, unknown> = extraFilter
|
||||
? {
|
||||
operator: "AND",
|
||||
conditions: [
|
||||
...(Object.keys(simple).length > 0 ? [simple] : []),
|
||||
extraFilter,
|
||||
],
|
||||
}
|
||||
: simple;
|
||||
// Pinned-first and the configured list order (#718) use the hasKeyword
|
||||
// sort comparator (RFC 8621 §4.4.2); every page of a view must use the
|
||||
// same sort or pagination tears, so the sort is built from settings the
|
||||
// same way on the first page, load-more and the push refresh.
|
||||
const built = await this.buildListSort(targetAccountId, pinnedFirst === true, order);
|
||||
const query = (sort: ReturnType<typeof buildEmailSort>) => this.request([
|
||||
["Email/query", {
|
||||
accountId: targetAccountId,
|
||||
filter,
|
||||
sort,
|
||||
limit,
|
||||
position,
|
||||
calculateTotal: true,
|
||||
}, "0"],
|
||||
["Email/get", {
|
||||
accountId: targetAccountId,
|
||||
"#ids": { resultOf: "0", name: "Email/query", path: "/ids" },
|
||||
properties: [...EMAIL_LIST_PROPERTIES],
|
||||
}, "1"],
|
||||
]);
|
||||
|
||||
let response = await query(built.sort);
|
||||
// A server that advertises nothing but refuses hasKeyword fails the whole
|
||||
// query. Remember that and retry once without the keyword comparators
|
||||
// rather than showing an empty folder.
|
||||
if (
|
||||
built.keywordSortSupported &&
|
||||
response.methodResponses?.[0]?.[0] === "error" &&
|
||||
(response.methodResponses[0][1] as { type?: string })?.type === "unsupportedSort"
|
||||
) {
|
||||
this.keywordSortUnsupported.add(targetAccountId);
|
||||
response = await query(buildEmailSort(order, { pinnedFirst, keywordSortSupported: false }));
|
||||
}
|
||||
|
||||
const queryResponse = response.methodResponses?.[0]?.[1];
|
||||
const getResponse = response.methodResponses?.[1]?.[1];
|
||||
|
||||
if (response.methodResponses?.[1]?.[0] === "Email/get" && getResponse) {
|
||||
const emails = (getResponse.list || []) as Email[];
|
||||
// Sort client-side as safety net - some servers may not honour
|
||||
// the query sort for large mailboxes without additional filters.
|
||||
// Must mirror the query sort, or it would undo the configured order.
|
||||
emails.sort(compareEmails(order, { pinnedFirst }));
|
||||
const total = queryResponse?.total || 0;
|
||||
const hasMore = computeHasMore(position, emails.length, total, limit);
|
||||
|
||||
if (accountId && accountId !== this.accountId) {
|
||||
namespaceMailboxIds(emails, accountId);
|
||||
}
|
||||
|
||||
// The Email collection state this page was read at; the store keeps
|
||||
// it so a later push can be applied with Email/changes.
|
||||
const state = typeof getResponse.state === 'string' ? getResponse.state : undefined;
|
||||
return { emails, hasMore, total, state };
|
||||
}
|
||||
|
||||
throw new Error(methodErrorMessage(response, 'Failed to get emails'));
|
||||
}
|
||||
|
||||
async getEmailsInMailbox(mailboxId: string): Promise<Email[]> {
|
||||
|
||||
@@ -122,3 +122,20 @@ describe('refreshCurrentMailbox merge', () => {
|
||||
expect(useEmailStore.getState().emails.map((e) => e.id)).toEqual(['a']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('refreshCurrentMailbox when the server fails', () => {
|
||||
it('keeps the list on screen instead of emptying it', async () => {
|
||||
useSettingsStore.setState({ emailsPerPage: 3 });
|
||||
const a = makeEmail('a');
|
||||
useEmailStore.setState({
|
||||
selectedMailbox: 'd', mailboxes: [draftsMailbox], accountMailboxes: {}, emails: [a], totalEmails: 1,
|
||||
});
|
||||
const client = { getEmails: vi.fn(async () => { throw new Error('backend down'); }) } as unknown as IJMAPClient;
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {});
|
||||
|
||||
await useEmailStore.getState().refreshCurrentMailbox(client);
|
||||
|
||||
expect(useEmailStore.getState().emails).toEqual([a]);
|
||||
expect(useEmailStore.getState().totalEmails).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -3923,8 +3923,9 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
|
||||
// So does an "All folders" search (#1082).
|
||||
const unscopedSearchChanged = isUnscopedSearchActive(get()) && anyEmailChanged;
|
||||
if (accountChanges?.Email || syncedAccountEmailState || tagViewChanged || unscopedSearchChanged) {
|
||||
// A delta that could not be read falls back to the full refresh.
|
||||
const applied = syncedAccountEmailState
|
||||
? await get().applyEmailDelta(client, syncedAccountEmailState)
|
||||
? await get().applyEmailDelta(client, syncedAccountEmailState).catch(() => false)
|
||||
: false;
|
||||
if (!applied) {
|
||||
await get().refreshCurrentMailbox(client);
|
||||
|
||||
Reference in New Issue
Block a user