From e64763b487c998e37eaf1d5f9773c278dc23bed1 Mon Sep 17 00:00:00 2001 From: sudacode Date: Mon, 17 Aug 2026 02:12:14 -0700 Subject: [PATCH] fix(stats): drop stale excluded-word sync notifications - Skip notifyServerSync for superseded excluded-word writes so an in-flight ack can't trigger a redundant aggregate recompute - Await hook teardown so React's scheduler drains before unmounting DOM globals in tests - Add regression test proving a slow, superseded aggregates refresh cannot overwrite the newest totals --- stats/src/hooks/useExcludedWords.test.ts | 5 +- stats/src/hooks/useExcludedWords.ts | 5 +- stats/src/hooks/useVocabulary.test.tsx | 72 +++++++++++++++++++++--- 3 files changed, 72 insertions(+), 10 deletions(-) diff --git a/stats/src/hooks/useExcludedWords.test.ts b/stats/src/hooks/useExcludedWords.test.ts index 41e0684c..f1192820 100644 --- a/stats/src/hooks/useExcludedWords.test.ts +++ b/stats/src/hooks/useExcludedWords.test.ts @@ -278,8 +278,9 @@ test('overlapping writes serialize so an older list cannot overwrite a newer edi JSON.stringify({ words: third }), ]); assert.deepEqual(getExcludedWordsSnapshot(), third); - assert.equal(syncs.length, 2); - assert.equal(syncs.at(-1), JSON.stringify(third)); + // Only the final revision notifies: the first write's acknowledgement was + // already obsolete, so it must not trigger an aggregate recomputation. + assert.deepEqual(syncs, [JSON.stringify(third)]); } finally { unsubscribe(); globalThis.fetch = originalFetch; diff --git a/stats/src/hooks/useExcludedWords.ts b/stats/src/hooks/useExcludedWords.ts index 5b4ede0d..814ae0da 100644 --- a/stats/src/hooks/useExcludedWords.ts +++ b/stats/src/hooks/useExcludedWords.ts @@ -142,7 +142,10 @@ export async function setExcludedWords(words: ExcludedWord[]): Promise { console.error('Failed to persist excluded words to stats database', error); throw error; } - notifyServerSync(); + // A newer edit arrived while this write was in flight, so the server state + // this acknowledges is already obsolete. Its own acknowledgement notifies + // with the newest list; skipping here avoids a wasted aggregate scan. + if (revision === writeRevision) notifyServerSync(); }); writeChain = write.catch(() => {}); return write; diff --git a/stats/src/hooks/useVocabulary.test.tsx b/stats/src/hooks/useVocabulary.test.tsx index a69204db..1010e1b8 100644 --- a/stats/src/hooks/useVocabulary.test.tsx +++ b/stats/src/hooks/useVocabulary.test.tsx @@ -123,7 +123,7 @@ interface Harness { flush: () => Promise; tick: (ms: number) => Promise; unmount: () => Promise; - teardown: () => void; + teardown: () => Promise; } async function mountHook(): Promise { @@ -170,9 +170,15 @@ async function mountHook(): Promise { root = null; }); }, - teardown: () => { - if (root) root.unmount(); + teardown: async () => { + await act(async () => { + root?.unmount(); + root = null; + }); clock.restore(); + // React's scheduler can still have deferred work queued; let it drain on + // a real timer while the DOM globals it reads are still installed. + await new Promise((resolve) => setTimeout(resolve, 0)); uninstallLocalStorage(); uninstallDom(); resetExcludedWordsStoreForTests(); @@ -242,7 +248,7 @@ test('aggregate failures retry with backoff, then surface an error that Retry cl assert.equal(harness.state().aggregatesError, null); assert.deepEqual(harness.state().summary, summaryFixture()); } finally { - harness.teardown(); + await harness.teardown(); restoreClient(); console.error = originalConsoleError; } @@ -274,7 +280,7 @@ test('charts poll while the backfill is pending and stop once it is ready', asyn await harness.tick(60_000); assert.equal(chartCalls, 3); } finally { - harness.teardown(); + await harness.teardown(); restoreClient(); } }); @@ -307,7 +313,7 @@ test('aggregates refetch after an exclusion edit is acknowledged by the server', assert.equal(summaryCalls, 2, 'totals must not keep counting the excluded word'); assert.equal(chartCalls, 2); } finally { - harness.teardown(); + await harness.teardown(); restoreClient(); } }); @@ -334,8 +340,60 @@ test('pending retries are cancelled when the tab unmounts', async () => { assert.equal(summaryCalls, 1, 'no retry may run after unmount'); } finally { - harness.teardown(); + await harness.teardown(); restoreClient(); console.error = originalConsoleError; } }); + +test('a slow response from a superseded refresh cannot replace the newest aggregates', async () => { + let summaryCalls = 0; + let releaseSuperseded: (() => void) | null = null; + const restoreClient = stubVocabularyClient({ + getVocabularySummary: async () => { + summaryCalls += 1; + const call = summaryCalls; + // The second call is the one that gets superseded while still in flight. + if (call === 2) { + await new Promise((resolve) => { + releaseSuperseded = resolve; + }); + } + return { ...summaryFixture(), uniqueWords: call }; + }, + getVocabularyCharts: async () => chartsFixture(), + }); + const harness = await mountHook(); + + try { + await harness.flush(); + assert.equal(harness.state().summary?.uniqueWords, 1); + + // First refresh stalls, then a second refresh supersedes it and resolves. + await act(async () => { + harness.state().refreshAggregates(); + }); + await harness.flush(); + await act(async () => { + harness.state().refreshAggregates(); + }); + await harness.flush(); + + assert.equal(summaryCalls, 3); + assert.equal(harness.state().summary?.uniqueWords, 3); + + const release = releaseSuperseded as (() => void) | null; + assert.ok(release, 'expected the superseded request to still be in flight'); + release(); + await harness.flush(); + + assert.equal( + harness.state().summary?.uniqueWords, + 3, + 'the superseded response must not overwrite the newest totals', + ); + } finally { + await harness.teardown(); + restoreClient(); + } +});