mirror of
https://github.com/bulwarkmail/native.git
synced 2026-10-31 16:06:32 +00:00
fix: stop re-counting every tag each time the sidebar opens
Every drawer open sent the whole set of tag count queries again (14 Email/query calls for 7 tags, twice that with a shared account), although nothing had changed since the last open. The counts now live in a small store, keyed by the login, the accounts and the tags they were fetched for. Opening the drawer only fetches when there are no counts for that key yet or when an Email change was reported for one of the accounts since (push or the poll fallback); a change that arrives while the drawer is open refreshes them there and then. Overlapping opens share one request, and a change that lands while the counts are in flight leaves them due for another fetch.
This commit is contained in:
@@ -32,7 +32,7 @@ import { jmapClient } from '../api/jmap-client';
|
||||
import {
|
||||
markMailboxAsRead, emptyMailbox, createMailbox, updateMailbox, deleteMailbox,
|
||||
} from '../api/email';
|
||||
import { fetchTagCounts, type TagCount } from '../api/tag-counts';
|
||||
import { useTagCountsStore } from '../stores/tag-counts-store';
|
||||
import { useNavigation } from '@react-navigation/native';
|
||||
import type { NativeStackNavigationProp } from '@react-navigation/native-stack';
|
||||
import type { RootStackParamList } from '../navigation/types';
|
||||
@@ -338,7 +338,9 @@ export default function SidebarDrawer({ visible, onClose }: SidebarDrawerProps)
|
||||
title: string; message?: string; initial?: string; confirmLabel: string; onSubmit: (v: string) => void;
|
||||
} | null>(null);
|
||||
const [busy, setBusy] = React.useState(false);
|
||||
const [tagCounts, setTagCounts] = React.useState<Map<string, TagCount>>(new Map());
|
||||
const tagCounts = useTagCountsStore((s) => s.counts);
|
||||
const tagCountsGeneration = useTagCountsStore((s) => s.generation);
|
||||
const ensureTagCounts = useTagCountsStore((s) => s.ensure);
|
||||
|
||||
React.useEffect(() => { if (!keywordsHydrated) void hydrateKeywords(); }, [keywordsHydrated, hydrateKeywords]);
|
||||
|
||||
@@ -429,23 +431,17 @@ export default function SidebarDrawer({ visible, onClose }: SidebarDrawerProps)
|
||||
onClose();
|
||||
}, [setFilters, onClose]);
|
||||
|
||||
// Tag counts: one query pair per tag and account, refreshed each time the
|
||||
// drawer opens with the section expanded. A tag view lists the tagged mail
|
||||
// of the own and the shared accounts, so its badge counts all of them
|
||||
// (#1038).
|
||||
// Tag counts: one query pair per tag and account. A tag view lists the
|
||||
// tagged mail of the own and the shared accounts, so its badge counts all
|
||||
// of them (#1038). They are cached until an Email change is reported
|
||||
// (PF6): opening the drawer costs no request when nothing changed, and
|
||||
// while it is open a change refreshes them.
|
||||
const tagAccountsKey = React.useMemo(() => spannedAccounts(mailboxes).join('|'), [mailboxes]);
|
||||
React.useEffect(() => {
|
||||
if (!visible || !tagsExpanded || keywordDefs.length === 0 || !jmapClient.isConnected) return;
|
||||
let cancelled = false;
|
||||
if (!visible || !tagsExpanded || keywordDefs.length === 0 || !activeAccountId || !jmapClient.isConnected) return;
|
||||
const accounts = tagAccountsKey.split('|').map((id) => id || undefined);
|
||||
fetchTagCounts(keywordDefs.map((k) => k.id), accounts)
|
||||
.then((counts) => {
|
||||
if (cancelled) return;
|
||||
setTagCounts(new Map(counts.map((tc) => [tc.id, tc])));
|
||||
})
|
||||
.catch(() => { /* counts are decoration */ });
|
||||
return () => { cancelled = true; };
|
||||
}, [visible, tagsExpanded, keywordDefs, tagAccountsKey]);
|
||||
void ensureTagCounts(activeAccountId, keywordDefs.map((k) => k.id), accounts);
|
||||
}, [visible, tagsExpanded, keywordDefs, tagAccountsKey, activeAccountId, tagCountsGeneration, ensureTagCounts]);
|
||||
|
||||
// ── Folder actions (long-press) ───────────────────────────────────────
|
||||
const refFor = (mb: Mailbox) => ({
|
||||
@@ -1101,7 +1097,7 @@ export default function SidebarDrawer({ visible, onClose }: SidebarDrawerProps)
|
||||
<Text style={styles.sectionHeaderText}>{t('sidebar.tags', 'Tags')}</Text>
|
||||
</Pressable>
|
||||
{tagsExpanded && keywordDefs.map((kw) => {
|
||||
const counts = tagCounts.get(kw.id);
|
||||
const counts = tagCounts[kw.id];
|
||||
const isSelected = filters.keyword === keywordToken(kw.id);
|
||||
const dot = c.tags[kw.color]?.dot ?? c.textMuted;
|
||||
return (
|
||||
|
||||
@@ -93,6 +93,7 @@ import * as settingsModule from '../settings-store';
|
||||
import {
|
||||
useEmailStore, viewerParamsForRow, deleteDestroysAcrossAccounts, accountIdOfRow,
|
||||
} from '../email-store';
|
||||
import { useTagCountsStore } from '../tag-counts-store';
|
||||
import type { Email, Mailbox } from '../../api/types';
|
||||
|
||||
const settings = (settingsModule as unknown as { __settings: Record<string, unknown> }).__settings;
|
||||
@@ -507,3 +508,19 @@ describe('tag view across the own and the team account (#1038)', () => {
|
||||
expect(queries[0][1]).toMatchObject({ accountId: 'team', filter: { inMailbox: 't-inbox' } });
|
||||
});
|
||||
});
|
||||
|
||||
describe('tag badges (PF6)', () => {
|
||||
it('go stale on an Email change in any account, not on a Mailbox change', async () => {
|
||||
const before = useTagCountsStore.getState().generation;
|
||||
|
||||
await useEmailStore.getState().handleStateChange({
|
||||
'@type': 'StateChange', changed: { team: { Mailbox: 'm2' } },
|
||||
} as never);
|
||||
expect(useTagCountsStore.getState().generation).toBe(before);
|
||||
|
||||
await useEmailStore.getState().handleStateChange({
|
||||
'@type': 'StateChange', changed: { team: { Email: 's2' } },
|
||||
} as never);
|
||||
expect(useTagCountsStore.getState().generation).toBe(before + 1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
|
||||
// PF6: every drawer open sent 14 Email/query calls for the tag badges, even
|
||||
// when nothing had changed. The counts are now cached and fetched again only
|
||||
// after an Email change was reported.
|
||||
|
||||
vi.mock('../../api/jmap-client', () => ({
|
||||
jmapClient: {
|
||||
accountId: 'c',
|
||||
getMaxCallsInRequest: () => 16,
|
||||
request: vi.fn(async (calls: Array<[string, Record<string, unknown>, string]>) => ({
|
||||
methodResponses: calls.map(([name, args, callId]) => [
|
||||
name,
|
||||
{ ids: [], total: 'operator' in (args.filter as object) ? 1 : 3 },
|
||||
callId,
|
||||
]),
|
||||
})),
|
||||
},
|
||||
}));
|
||||
|
||||
import { jmapClient } from '../../api/jmap-client';
|
||||
import { useTagCountsStore } from '../tag-counts-store';
|
||||
|
||||
const mockRequest = jmapClient.request as ReturnType<typeof vi.fn>;
|
||||
const TAGS = ['red', 'blue', 'green', 'work', 'home', 'todo', 'later'];
|
||||
|
||||
beforeEach(() => {
|
||||
mockRequest.mockClear();
|
||||
useTagCountsStore.setState({ counts: {}, login: null, key: null, generation: 0, fetchedAt: -1 });
|
||||
});
|
||||
|
||||
describe('tag counts cache (PF6)', () => {
|
||||
it('fetches the 7 tags in one request, then serves the cache', async () => {
|
||||
const { ensure } = useTagCountsStore.getState();
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
|
||||
expect(mockRequest).toHaveBeenCalledTimes(1);
|
||||
expect(mockRequest.mock.calls[0][0]).toHaveLength(14);
|
||||
expect(useTagCountsStore.getState().counts.red).toEqual({ id: 'red', total: 3, unread: 1 });
|
||||
|
||||
// The drawer opens again: nothing changed, nothing is sent.
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
expect(mockRequest).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('fetches again after an Email change', async () => {
|
||||
const { ensure, invalidate } = useTagCountsStore.getState();
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
invalidate();
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
expect(mockRequest).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('shares one request between overlapping opens', async () => {
|
||||
const { ensure } = useTagCountsStore.getState();
|
||||
await Promise.all([ensure('me', TAGS, [undefined]), ensure('me', TAGS, [undefined])]);
|
||||
expect(mockRequest).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('stays stale when a change lands while the counts are in flight', async () => {
|
||||
const { ensure, invalidate } = useTagCountsStore.getState();
|
||||
const first = ensure('me', TAGS, [undefined]);
|
||||
invalidate();
|
||||
await first;
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
expect(mockRequest).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('fetches for other tags, other accounts or another login', async () => {
|
||||
const { ensure } = useTagCountsStore.getState();
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
await ensure('me', [...TAGS, 'new'], [undefined]);
|
||||
await ensure('me', [...TAGS, 'new'], [undefined, 'team']);
|
||||
expect(mockRequest).toHaveBeenCalledTimes(1 + 1 + 2);
|
||||
|
||||
// Another login's badges are dropped at once, not shown meanwhile.
|
||||
const other = ensure('someone-else', TAGS, [undefined]);
|
||||
expect(useTagCountsStore.getState().counts).toEqual({});
|
||||
await other;
|
||||
expect(useTagCountsStore.getState().counts.red).toBeDefined();
|
||||
});
|
||||
|
||||
it('keeps the old counts when the fetch fails and retries next time', async () => {
|
||||
const { ensure, invalidate } = useTagCountsStore.getState();
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
invalidate();
|
||||
mockRequest.mockRejectedValueOnce(new Error('offline'));
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
expect(useTagCountsStore.getState().counts.red).toEqual({ id: 'red', total: 3, unread: 1 });
|
||||
|
||||
await ensure('me', TAGS, [undefined]);
|
||||
expect(mockRequest).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
});
|
||||
@@ -48,6 +48,7 @@ import { t } from './locale-store';
|
||||
import { useSettingsStore } from './settings-store';
|
||||
import { useOfflineCacheStore } from './offline-cache-store';
|
||||
import { useOutboxStore, applyOrQueue, applyOrQueueBatch, type OutboxOp } from './outbox-store';
|
||||
import { useTagCountsStore } from './tag-counts-store';
|
||||
|
||||
// ── Refresh coalescing ─────────────────────────────────────────────────
|
||||
// Push events, mount effects and post-action follow-ups all call
|
||||
@@ -1159,12 +1160,16 @@ export const useEmailStore = create<EmailState>()(
|
||||
|
||||
let mailboxChanged = false;
|
||||
let emailChanged = false;
|
||||
let tagCountsChanged = false;
|
||||
for (const [accountId, accountChanges] of Object.entries(change.changed ?? {})) {
|
||||
if (!known.has(accountId) || !accountChanges) continue;
|
||||
if ('Mailbox' in accountChanges) mailboxChanged = true;
|
||||
// The sidebar tag badges count every account (PF6, #1038).
|
||||
if ('Email' in accountChanges) tagCountsChanged = true;
|
||||
if (accountId !== currentAccountId && !spanning) continue;
|
||||
if ('Email' in accountChanges || 'EmailDelivery' in accountChanges) emailChanged = true;
|
||||
}
|
||||
if (tagCountsChanged) useTagCountsStore.getState().invalidate();
|
||||
if (!mailboxChanged && !emailChanged) return;
|
||||
|
||||
if (mailboxChanged) {
|
||||
|
||||
@@ -0,0 +1,62 @@
|
||||
import { create } from 'zustand';
|
||||
import { fetchTagCounts, type TagCount } from '../api/tag-counts';
|
||||
|
||||
// Sidebar tag badges (PF6). Every drawer open used to send the full set of
|
||||
// Email/query pairs again, one pair per tag and account, although nothing
|
||||
// had changed. The counts are now kept for the login, the accounts and the
|
||||
// tags they were fetched for, and fetched again only once an Email change
|
||||
// was reported for one of those accounts (push, or the poll fallback), or
|
||||
// when the login, the accounts or the tags differ.
|
||||
|
||||
interface TagCountsState {
|
||||
/** Tag id → counts, summed over the accounts they were fetched for. */
|
||||
counts: Record<string, TagCount>;
|
||||
/** Login the counts belong to; another login's are dropped, not shown. */
|
||||
login: string | null;
|
||||
/** The login, accounts and tags the counts were fetched for. */
|
||||
key: string | null;
|
||||
/** Bumped by every reported Email change. */
|
||||
generation: number;
|
||||
/** `generation` the counts were fetched at. */
|
||||
fetchedAt: number;
|
||||
/** Fetch the counts unless the cached ones are current. */
|
||||
ensure: (login: string, tagIds: string[], accountIds: Array<string | undefined>) => Promise<void>;
|
||||
/** An Email change arrived: the next `ensure` fetches again. */
|
||||
invalidate: () => void;
|
||||
}
|
||||
|
||||
let inflight: { key: string; generation: number; seq: number; promise: Promise<void> } | null = null;
|
||||
let lastSeq = 0;
|
||||
|
||||
export const useTagCountsStore = create<TagCountsState>((set, get) => ({
|
||||
counts: {},
|
||||
login: null,
|
||||
key: null,
|
||||
generation: 0,
|
||||
fetchedAt: -1,
|
||||
|
||||
ensure: (login, tagIds, accountIds) => {
|
||||
const key = `${login}|${accountIds.map((id) => id ?? '').join(',')}|${tagIds.join(',')}`;
|
||||
if (get().login !== login) set({ counts: {}, login, key: null });
|
||||
const { generation } = get();
|
||||
if (get().key === key && get().fetchedAt === generation) return Promise.resolve();
|
||||
if (inflight && inflight.key === key && inflight.generation === generation) return inflight.promise;
|
||||
const seq = ++lastSeq;
|
||||
const promise = (async () => {
|
||||
try {
|
||||
const list = await fetchTagCounts(tagIds, accountIds);
|
||||
// A later ensure (another login, other tags) supersedes this one.
|
||||
if (inflight?.seq !== seq || get().login !== login) return;
|
||||
set({ counts: Object.fromEntries(list.map((c) => [c.id, c])), key, fetchedAt: generation });
|
||||
} catch {
|
||||
// Counts are decoration: keep the old ones and try again next time.
|
||||
} finally {
|
||||
if (inflight?.seq === seq) inflight = null;
|
||||
}
|
||||
})();
|
||||
inflight = { key, generation, seq, promise };
|
||||
return promise;
|
||||
},
|
||||
|
||||
invalidate: () => set({ generation: get().generation + 1 }),
|
||||
}));
|
||||
Reference in New Issue
Block a user