mirror of
https://github.com/bulwarkmail/webmail.git
synced 2026-11-01 16:12:02 +00:00
fix(search): guard quick search against stale responses
Quick search fired one request per keystroke with no AbortController and no staleness check, so whichever response landed last won regardless of which query it belonged to. Typing an address slowly showed hits belonging to a short prefix while the box held the full string; retyping the same string in one go correctly showed none, which reads as "search works sometimes". advancedSearch already guarded every write with controller.signal.aborted. This gives searchEmails the same treatment, reusing the existing searchAbortController so the two search paths supersede each other rather than race. The catch branch is guarded too: without it a superseded request that rejects clears results belonging to the current query and surfaces a misleading error. Adds stores/__tests__/email-store-search-race.test.ts with three cases, all of which fail on main and pass with this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
58ddd01cf7
commit
88712994de
@@ -0,0 +1,134 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { useEmailStore } from '../email-store';
|
||||
import { useAuthStore } from '../auth-store';
|
||||
import { useSettingsStore } from '@/stores/settings-store';
|
||||
import { DEFAULT_SEARCH_FILTERS } from '@/lib/jmap/search-utils';
|
||||
import type { Email, Mailbox } from '@/lib/jmap/types';
|
||||
import type { IJMAPClient } from '@/lib/jmap/client-interface';
|
||||
|
||||
// Quick search fired an unguarded request per keystroke: no AbortController and
|
||||
// no staleness check, so whichever response landed LAST won regardless of which
|
||||
// query it belonged to. advancedSearch already guarded every write with
|
||||
// `controller.signal.aborted`; searchEmails did not.
|
||||
//
|
||||
// The visible symptom is nasty because it looks like a server bug: type an
|
||||
// address slowly and a short-prefix response arrives after the full-query one
|
||||
// and repaints the list, so the box and the results disagree. Retype the same
|
||||
// string in one go and you get the (correct) empty result, which reads as "the
|
||||
// same search works sometimes and not others".
|
||||
|
||||
function makeMailbox(overrides: Partial<Mailbox> = {}): Mailbox {
|
||||
return {
|
||||
id: 'inbox',
|
||||
name: 'Inbox',
|
||||
sortOrder: 0,
|
||||
totalEmails: 0,
|
||||
unreadEmails: 0,
|
||||
totalThreads: 0,
|
||||
unreadThreads: 0,
|
||||
isSubscribed: true,
|
||||
isShared: false,
|
||||
...overrides,
|
||||
} as Mailbox;
|
||||
}
|
||||
|
||||
function makeEmail(id: string): Email {
|
||||
return { id, subject: id, receivedAt: '2026-01-01T00:00:00Z', mailboxIds: { inbox: true } } as unknown as Email;
|
||||
}
|
||||
|
||||
describe('quick search staleness', () => {
|
||||
let client: IJMAPClient;
|
||||
type Pending = {
|
||||
resolve: (value: { emails: Email[]; hasMore: boolean; total: number }) => void;
|
||||
reject: (reason: unknown) => void;
|
||||
};
|
||||
let pending: Pending[];
|
||||
|
||||
beforeEach(() => {
|
||||
pending = [];
|
||||
|
||||
client = {
|
||||
// Hand back a promise per call that the test settles by hand, so the
|
||||
// responses can be made to arrive out of order.
|
||||
searchEmails: vi.fn().mockImplementation(
|
||||
() => new Promise((resolve, reject) => { pending.push({ resolve, reject }); })
|
||||
),
|
||||
advancedSearchEmails: vi.fn().mockResolvedValue({ emails: [], hasMore: false, total: 0 }),
|
||||
getSomeEmails: vi.fn().mockResolvedValue([]),
|
||||
} as unknown as IJMAPClient;
|
||||
|
||||
useAuthStore.setState({
|
||||
activeAccountId: 'account-a',
|
||||
getClientForAccount: (id: string) => (id === 'account-a' ? client : undefined) as never,
|
||||
} as never);
|
||||
|
||||
useSettingsStore.setState({ emailsPerPage: 50 } as never);
|
||||
|
||||
useEmailStore.setState({
|
||||
isUnifiedView: false,
|
||||
unifiedRole: null,
|
||||
crossView: null,
|
||||
viewingAccountId: null,
|
||||
selectedMailbox: 'inbox',
|
||||
searchMailboxId: '',
|
||||
searchQuery: '',
|
||||
searchFilters: { ...DEFAULT_SEARCH_FILTERS },
|
||||
searchAbortController: null,
|
||||
emails: [],
|
||||
mailboxes: [makeMailbox({ id: 'inbox', role: 'inbox' })],
|
||||
accountMailboxes: {},
|
||||
});
|
||||
});
|
||||
|
||||
it('discards a slow response once a newer query has been started', async () => {
|
||||
// Keystroke 1 goes out and stalls.
|
||||
const first = useEmailStore.getState().searchEmails(client, 'qeti');
|
||||
// Keystroke 2 goes out before the first came back.
|
||||
const second = useEmailStore.getState().searchEmails(client, 'qeti@smartchoice.ge');
|
||||
|
||||
expect(client.searchEmails).toHaveBeenCalledTimes(2);
|
||||
|
||||
// The newer query legitimately matches nothing.
|
||||
pending[1].resolve({ emails: [], hasMore: false, total: 0 });
|
||||
await second;
|
||||
|
||||
// The older, broader query now answers late with hits.
|
||||
pending[0].resolve({ emails: [makeEmail('stale-1'), makeEmail('stale-2')], hasMore: false, total: 2 });
|
||||
await first;
|
||||
|
||||
const state = useEmailStore.getState();
|
||||
expect(state.searchQuery).toBe('qeti@smartchoice.ge');
|
||||
// Without the guard the late response repaints the list and the user sees
|
||||
// two results for a query that matched none.
|
||||
expect(state.emails).toEqual([]);
|
||||
expect(state.totalEmails).toBe(0);
|
||||
});
|
||||
|
||||
it('aborts the in-flight request when a new search starts', async () => {
|
||||
const first = useEmailStore.getState().searchEmails(client, 'qeti');
|
||||
const controllerForFirst = useEmailStore.getState().searchAbortController;
|
||||
expect(controllerForFirst?.signal.aborted).toBe(false);
|
||||
|
||||
const second = useEmailStore.getState().searchEmails(client, 'qeti@smartchoice.ge');
|
||||
expect(controllerForFirst?.signal.aborted).toBe(true);
|
||||
|
||||
pending.forEach(p => p.resolve({ emails: [], hasMore: false, total: 0 }));
|
||||
await Promise.all([first, second]);
|
||||
});
|
||||
|
||||
it('does not let a stale failure wipe the current results', async () => {
|
||||
const first = useEmailStore.getState().searchEmails(client, 'qeti');
|
||||
const second = useEmailStore.getState().searchEmails(client, 'qeti@smartchoice.ge');
|
||||
|
||||
pending[1].resolve({ emails: [makeEmail('good-1')], hasMore: false, total: 1 });
|
||||
await second;
|
||||
|
||||
// The superseded request rejects after the fact.
|
||||
pending[0].reject(new Error("network blip"));
|
||||
await first;
|
||||
|
||||
const state = useEmailStore.getState();
|
||||
expect(state.error).toBeNull();
|
||||
expect(state.emails.map(e => e.id)).toEqual(['good-1']);
|
||||
});
|
||||
});
|
||||
+27
-5
@@ -2259,7 +2259,22 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
|
||||
},
|
||||
|
||||
searchEmails: async (client, query) => {
|
||||
set({ isLoading: true, error: null, searchQuery: query, emails: [], hasMoreEmails: false, totalEmails: 0 }); // Clear emails for loading state
|
||||
const { searchAbortController } = get();
|
||||
|
||||
if (searchAbortController) {
|
||||
searchAbortController.abort();
|
||||
}
|
||||
|
||||
const controller = new AbortController();
|
||||
set({
|
||||
isLoading: true,
|
||||
error: null,
|
||||
searchQuery: query,
|
||||
emails: [],
|
||||
hasMoreEmails: false,
|
||||
totalEmails: 0,
|
||||
searchAbortController: controller,
|
||||
}); // Clear emails for loading state
|
||||
try {
|
||||
const { isUnifiedView, unifiedRole, crossView, searchMailboxId, searchFilters } = get();
|
||||
const emailsPerPage = useSettingsStore.getState().emailsPerPage;
|
||||
@@ -2308,10 +2323,14 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
|
||||
result.total += newEmails.length;
|
||||
}
|
||||
|
||||
if (controller.signal.aborted) return;
|
||||
|
||||
const externals = await emailHooks.onProvideSearchResults.transform([] as ExternalSearchResult[], {
|
||||
query,
|
||||
filters: searchFilters
|
||||
query,
|
||||
filters: searchFilters
|
||||
});
|
||||
|
||||
if (controller.signal.aborted) return;
|
||||
result.emails = await emailHooks.onEmailsFetched.transform(result.emails);
|
||||
set({
|
||||
emails: annotateScheduledEmails(result.emails, get().scheduledSubmissionByEmailId),
|
||||
@@ -2319,16 +2338,19 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
|
||||
hasMoreEmails: result.hasMore,
|
||||
totalEmails: result.total,
|
||||
isLoading: false,
|
||||
...(unifiedErrors ? { unifiedErrors } : {})
|
||||
searchAbortController: null,
|
||||
...(unifiedErrors ? { unifiedErrors } : {})
|
||||
});
|
||||
} catch (error) {
|
||||
if (controller.signal.aborted) return;
|
||||
set({
|
||||
error: error instanceof Error ? error.message : "Failed to search emails",
|
||||
isLoading: false,
|
||||
emails: [],
|
||||
externalSearchResults: [],
|
||||
hasMoreEmails: false,
|
||||
totalEmails: 0
|
||||
totalEmails: 0,
|
||||
searchAbortController: null,
|
||||
});
|
||||
}
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user