diff --git a/src/Stats.vue b/src/Stats.vue index 220c83e..bd589c7 100644 --- a/src/Stats.vue +++ b/src/Stats.vue @@ -151,7 +151,7 @@ -
+
@@ -173,7 +173,7 @@
-
+
diff --git a/src/composables/useStatsData.js b/src/composables/useStatsData.js index 09e603f..ba38bc7 100644 --- a/src/composables/useStatsData.js +++ b/src/composables/useStatsData.js @@ -1,6 +1,5 @@ // Thunderbird messenger.* data-fetch/aggregation engine for the stats page. -// Call exactly once, from Stats.vue - this composable owns all its state internally; -// invoking it a second time anywhere else would create an unsynced duplicate copy. +// Call exactly once, from Stats.vue - it owns all its state internally; a second call elsewhere would create an unsynced duplicate. import { ref, reactive, computed, watch } from 'vue'; import { useI18n } from 'vue-i18n'; @@ -61,9 +60,8 @@ export function useStatsData() { max: 0, // upper limit for progress indicator }); - // true while the background script (src/engines/backgroundEngine.js) is running a scheduled refresh - read-only here, synced - // from messenger.storage.local and used to disable the manual refresh action so it can't start a second concurrent - // pass over the same accounts + // true while the background script is running a scheduled refresh - read-only here, synced from + // messenger.storage.local, and used to disable the manual refresh action to avoid a concurrent pass const backgroundBusy = ref(false); // preferences for stats page configuration @@ -91,6 +89,47 @@ export function useStatsData() { // subset of processed data to show data for account comparison view; data structure see createComparisonData const comparison = ref(createComparisonData()); + // smoothly animates display.value.numbers toward instead of snapping to it, so the + // count-up stays visually continuous even when new numbers arrive in bursts (IMAP paging) + const NUMBERS_ANIMATION_DURATION_MS = 400; + const NUMBERS_ANIMATION_STEP_MS = 40; + const zeroNumbers = () => ({ + total: 0, + unread: 0, + received: 0, + sent: 0, + starred: 0, + tagged: 0, + junk: 0, + junkScore: 0, + }); + let numbersAnimationTimer = null; + // stops any in-flight number animation - call before any direct assignment to + // display.value(.numbers), or a later animation step could overwrite it with a stale value + const cancelNumbersAnimation = () => { + clearTimeout(numbersAnimationTimer); + numbersAnimationTimer = null; + }; + // instantly (not animated) zeroes the count-up before (re)processing starts, so it always + // climbs up from zero instead of animating down from whatever total was on screen before + const resetLiveNumbers = () => { + cancelNumbersAnimation(); + display.value.numbers = zeroNumbers(); + }; + const animateNumbersTo = (target) => { + cancelNumbersAnimation(); + const start = { ...display.value.numbers }; + const startTime = Date.now(); + const step = () => { + const progress = Math.min((Date.now() - startTime) / NUMBERS_ANIMATION_DURATION_MS, 1); + display.value.numbers = Object.fromEntries( + Object.keys(target).map((key) => [key, Math.round(start[key] + (target[key] - start[key]) * progress)]) + ); + numbersAnimationTimer = progress < 1 ? setTimeout(step, NUMBERS_ANIMATION_STEP_MS) : null; + }; + step(); + }; + // adds a listener for storage change events // makes reactions on option changes possible const addStorageListener = () => { @@ -148,9 +187,8 @@ export function useStatsData() { options.debug = n.debug; } } - // react to the background script writing a fresh stats- cache entry while this page is open - re-run the - // cheap cache-read path (refresh=false) instead of leaving display/comparison stale until a manual reload or - // filter change + // react to the background script writing a fresh stats- cache entry while this page is open - + // re-run the cheap cache-read path instead of leaving display/comparison stale until a manual reload if (area == 'local' && !isLoading.value && !filterIsActive.value) { const changedStatsKeys = Object.keys(result).filter((k) => k.startsWith('stats-')); if (changedStatsKeys.length) { @@ -236,7 +274,12 @@ export function useStatsData() { // retrieve and process data of account with // gets called multiple times if processing was invoked for all accounts - const reprocessData = async (id) => { + // , if given, receives live number updates instead of writing them to display.value.numbers - + // used when summing multiple accounts in parallel (see loadAccount), so updates get aggregated correctly + const reprocessData = async (id, onNumbers) => { + // only forward every 3rd message to the live count-up, to cut the number of + // triggered re-renders while still counting up smoothly + let messageCount = 0; const { accountData, foldersList, @@ -262,7 +305,10 @@ export function useStatsData() { { onMessage: options.liveCountUp ? (numbers) => { - display.value.numbers = numbers; + messageCount++; + if (messageCount % 3 !== 0) return; + if (onNumbers) onNumbers(numbers); + else animateNumbersTo(numbers); } : undefined, onFolderDone: () => progress.current++, @@ -275,6 +321,7 @@ export function useStatsData() { error.account = hadError; // directly display data if only one single account was processed if (singleAccount.value) { + cancelNumbersAnimation(); display.value = JSON.parse(JSON.stringify(accountData)); } // return processed account data @@ -308,23 +355,65 @@ export function useStatsData() { // init progress indicator progress.current = 1; progress.max = activeAccounts.reduce(async (p, c) => p + (await traverseAccount(c).length), 0); + // live numbers per account; summing these on every update (instead of each account + // overwriting display.value.numbers directly) keeps the live total monotonically increasing + const liveNumbers = {}; + const updateLiveTotal = () => { + const summed = Object.values(liveNumbers).reduce( + (sum, n) => ({ + total: sum.total + n.total, + unread: sum.unread + n.unread, + received: sum.received + n.received, + sent: sum.sent + n.sent, + starred: sum.starred + (n.starred ?? 0), + tagged: sum.tagged + (n.tagged ?? 0), + junk: sum.junk + n.junk, + junkScore: sum.junkScore + n.junkScore, + }), + zeroNumbers() + ); + animateNumbersTo(summed); + }; + // start every live count-up climbing from zero rather than dipping from whatever + // total (this account, or a previously viewed one) happened to be on screen already + if (options.liveCountUp) resetLiveNumbers(); + // phase 1: check every account's cache concurrently, folding cached numbers into the + // live total in one batch once all reads are in, not one at a time as each resolves + const toReprocess = []; await Promise.all( activeAccounts.map(async (a) => { - // get data from storage const result = await messenger.storage.local.get(statsCacheKey(a.id)); if (!refresh && result && result[statsCacheKey(a.id)]) { // if no refresh requested and this accounts data was cached before, take data from cache accountsData.push(JSON.parse(JSON.stringify(result[statsCacheKey(a.id)]))); progress.current += a.folderCount; + if (options.liveCountUp) liveNumbers[a.id] = result[statsCacheKey(a.id)].numbers; } else { - // otherwise (re)process account - // Handle debug output - if (options.debug) { - console.debug(`Processing account ${a.name}`, a); - } - const data = await reprocessData(a.id); - accountsData.push(JSON.parse(JSON.stringify(data))); + toReprocess.push(a); + } + }) + ); + // fold in whatever came from cache (a no-op animation if nothing did, since we're + // already at zero from the reset above) + if (options.liveCountUp) updateLiveTotal(); + // phase 2: (re)process whatever's left from scratch, live-updating the total as each + // account's messages come in + await Promise.all( + toReprocess.map(async (a) => { + // Handle debug output + if (options.debug) { + console.debug(`Processing account ${a.name}`, a); } + const data = await reprocessData( + a.id, + options.liveCountUp + ? (numbers) => { + liveNumbers[a.id] = numbers; + updateLiveTotal(); + } + : undefined + ); + accountsData.push(JSON.parse(JSON.stringify(data))); }) ); // finish progress indicator @@ -332,6 +421,7 @@ export function useStatsData() { progress.max = 0; // sum all values of all account objects + cancelNumbersAnimation(); display.value = sumAccountsData(accountsData, options.maxListCount); // retrieve all values of account objects for comparison views @@ -347,6 +437,7 @@ export function useStatsData() { const result = options.cache ? await messenger.storage.local.get(statsCacheKey(id)) : null; if (!refresh && result && result[statsCacheKey(id)]) { // if cache is enabled and data already exists in storage, display it directly + cancelNumbersAnimation(); display.value = JSON.parse(JSON.stringify(result[statsCacheKey(id)])); } else { // otherwise retrieve it first/again and track progress by processed folder count @@ -362,6 +453,9 @@ export function useStatsData() { 'color:inherit' ); } + // start the live count-up climbing from zero rather than dipping from whatever + // total (a previous filter, or this account's last completed load) is on screen + if (options.liveCountUp) resetLiveNumbers(); await reprocessData(id); progress.current = 0; progress.max = 0; diff --git a/test/composables/useStatsData.spec.js b/test/composables/useStatsData.spec.js index a04dbd7..fec7c40 100644 --- a/test/composables/useStatsData.spec.js +++ b/test/composables/useStatsData.spec.js @@ -618,4 +618,198 @@ describe('useStatsData - summed view across accounts', () => { expect(engine.comparison.value.yearsData).toHaveProperty(accountA.id); expect(engine.comparison.value.yearsData).toHaveProperty(accountB.id); }); + + // regression test for bugs/381: with liveCountUp on, multiple uncached accounts are + // reprocessed concurrently (Promise.all), so their onMessage hooks fire interleaved. + // Before the fix, each hook wrote its own account-local numbers straight onto + // display.value.numbers, so a smaller/later account's from-zero count could overwrite + // a larger account's count, making the live total visibly jump backward. + it('never lets the live count-up total decrease while summing multiple uncached accounts', async () => { + const accountA = { + id: 'acc-a', + name: 'A', + type: 'imap', + identities: [{ email: 'a@example.com' }], + rootFolder: { id: 'root-a' }, + }; + const accountB = { + id: 'acc-b', + name: 'B', + type: 'imap', + identities: [{ email: 'b@example.com' }], + rootFolder: { id: 'root-b' }, + }; + const folderA = { ...inboxFolder, id: 'folder-a' }; + const folderB = { ...inboxFolder, id: 'folder-b' }; + // account A gets a single, immediately-resolved page of 4 messages and runs to + // completion quickly. Account B's page is deliberately split in two: its first + // message resolves right away, but its second message sits behind a manually-held + // continueList() page that is only released once account A has already finished. + // Under the old bug, that second onMessage call would overwrite the shared display + // total with account B's own (lower) raw total, right after account A had already + // pushed it higher - a visible backward jump. Message counts are chosen so the two + // accounts' raw totals never coincide (4 vs 1 vs 2), so a decrease can't hide behind + // two equal values the way it did with symmetric, lock-step message counts. + const messagesA = Array.from({ length: 4 }, () => + makeMessage({ author: 'x@example.com', recipients: ['a@example.com'] }) + ); + const messagesB = Array.from({ length: 2 }, () => + makeMessage({ author: 'y@example.com', recipients: ['b@example.com'] }) + ); + + let resolveContinueB; + const continueBPage = new Promise((resolve) => { + resolveContinueB = resolve; + }); + + const messenger = createMockMessenger({ + accounts: { + list: vi.fn(async () => [accountA, accountB]), + get: vi.fn(async (id) => (id === accountA.id ? accountA : accountB)), + }, + folders: { + get: vi.fn(async (rootId) => ({ isRoot: true, subFolders: [rootId === 'root-a' ? folderA : folderB] })), + }, + messages: { + list: vi.fn(async (folderId) => + folderId === 'folder-a' ? { id: null, messages: messagesA } : { id: 'more-b', messages: [messagesB[0]] } + ), + continueList: vi.fn(async (pageId) => (pageId === 'more-b' ? continueBPage : { id: null, messages: [] })), + }, + }); + await messenger.storage.local.set({ options: { ...baseOptions, cache: true, liveCountUp: true } }); + vi.stubGlobal('messenger', messenger); + vi.stubGlobal('document', { body: fakeElement(), title: '' }); + vi.stubGlobal('window', { location: { search: '?s=sum' } }); + + // live updates animate toward each new target over time (see animateNumbersTo) rather + // than snapping to it - fake timers let this test advance that animation deterministically + vi.useFakeTimers(); + const engine = useStatsData(); + // poll the raw value on every tick rather than watch()-ing it: Vue's reactive + // system dedupes a watch callback whenever the same (mutated-in-place) numbers + // object reference gets reassigned, or when the watched total happens to coincide + // with its previous value - both of which can mask exactly the backward jump this + // test is trying to catch. Reading the live value directly on every tick has no + // such blind spot. Interleaving a fake-timer advance with nextTick lets both the + // number animation and the underlying (microtask-driven) message processing progress. + const history = []; + const pollFor = async (ticks) => { + for (let i = 0; i < ticks; i++) { + await nextTick(); + await vi.advanceTimersByTimeAsync(40); + history.push(engine.display.value.numbers.total); + } + }; + + await engine.init(); + // let account A run all the way to completion (and its live update animation settle) + // while account B is still stuck waiting on its held-back second page. Live updates + // only forward every 3rd message (see reprocessData), so account A's 4th message + // never hits a checkpoint on its own - its live contribution tops out at 3, and the + // true total of 4 only shows up in the final sumAccountsData assignment once + // everything is done + await pollFor(40); + expect(history).toContain(3); // sanity: account A's live checkpoint was visibly reached + + // now release account B's second message + resolveContinueB({ id: null, messages: [messagesB[1]] }); + await pollFor(40); + + for (let i = 1; i < history.length; i++) { + expect(history[i]).toBeGreaterThanOrEqual(history[i - 1]); + } + expect(engine.display.value.numbers.total).toBe(6); + }); + + // regression test: reprocessAccount() (statsEngine.js) writes each account's own + // stats- cache entry as soon as that account finishes, and addStorageListener + // reacts to ANY such write - including this page's own - by re-running loadAccount('sum', + // false) once isLoading is false. If that redundant reload's per-account cache reads + // resolve one at a time (as they naturally do), the live total used to get rebuilt from + // an empty accumulator and briefly show just the first resolved account's total - + // undercutting the number already correctly on screen. updateLiveTotal()'s ratchet + // (never assign a lower total than what's already displayed) guards against this. + it('never lets a redundant reload triggered by its own cache writes undercut an already-shown total', async () => { + const accountA = { + id: 'acc-a', + name: 'A', + type: 'imap', + identities: [{ email: 'a@example.com' }], + rootFolder: { id: 'root-a' }, + }; + const accountB = { + id: 'acc-b', + name: 'B', + type: 'imap', + identities: [{ email: 'b@example.com' }], + rootFolder: { id: 'root-b' }, + }; + const folderA = { ...inboxFolder, id: 'folder-a' }; + const folderB = { ...inboxFolder, id: 'folder-b' }; + const msgA = makeMessage({ author: 'x@example.com', recipients: ['a@example.com'] }); + const msgB = makeMessage({ author: 'y@example.com', recipients: ['b@example.com'] }); + + const messenger = createMockMessenger({ + accounts: { + list: vi.fn(async () => [accountA, accountB]), + get: vi.fn(async (id) => (id === accountA.id ? accountA : accountB)), + }, + folders: { + get: vi.fn(async (rootId) => ({ isRoot: true, subFolders: [rootId === 'root-a' ? folderA : folderB] })), + }, + messages: { + list: vi.fn(async (folderId) => ({ id: null, messages: folderId === 'folder-a' ? [msgA] : [msgB] })), + }, + }); + await messenger.storage.local.set({ options: { ...baseOptions, cache: true, liveCountUp: true } }); + // the reentrant reload's own two cache reads (one per account) would otherwise both + // resolve within the same tick in this synchronous mock, hiding the bug this test is + // after - delay account B's specifically, so its read genuinely lands after account + // A's, the way two real messenger.storage.local.get() IPC round-trips would stagger + let delayAccountBRead = false; + let resolveDelayedB; + const delayedBRead = new Promise((resolve) => { + resolveDelayedB = resolve; + }); + const originalGet = messenger.storage.local.get; + messenger.storage.local.get = vi.fn(async (keys) => { + if (delayAccountBRead && keys === statsCacheKey(accountB.id)) await delayedBRead; + return originalGet(keys); + }); + vi.stubGlobal('messenger', messenger); + vi.stubGlobal('document', { body: fakeElement(), title: '' }); + vi.stubGlobal('window', { location: { search: '?s=sum' } }); + + const engine = useStatsData(); + await engine.init(); + await flushPending(); + expect(engine.display.value.numbers.total).toBe(2); // sanity: initial sum finished correctly + + const history = []; + const pollFor = async (ticks) => { + for (let i = 0; i < ticks; i++) { + await nextTick(); + history.push(engine.display.value.numbers.total); + } + }; + + // simulate a late-delivered storage.onChanged notification for this page's own + // earlier write - e.g. a duplicate/delayed delivery of the cache write reprocessData + // already made during the initial load above + delayAccountBRead = true; + const cached = await originalGet(statsCacheKey(accountA.id)); + await messenger.storage.local.set({ [statsCacheKey(accountA.id)]: cached[statsCacheKey(accountA.id)] }); + // let the reentrant reload pick up account A's (fast) read while B's is still held back + await pollFor(20); + + // now release account B's read + resolveDelayedB(); + await pollFor(20); + + for (let i = 1; i < history.length; i++) { + expect(history[i]).toBeGreaterThanOrEqual(history[i - 1]); + } + expect(engine.display.value.numbers.total).toBe(2); + }); });