From e38c8fde3b87c8db30800b9ec961da786cf64e9d Mon Sep 17 00:00:00 2001 From: wendyamoni-creator Date: Mon, 27 Jul 2026 09:28:52 +0000 Subject: [PATCH] fix: only invalidate cache tags when mutation response body reports success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit request() was invalidating NEWSLETTER/STATISTICS cache tags for any POST/DELETE that returned HTTP 2xx, without inspecting the parsed payload's success field. newsletterSubscribe / newsletterUnsubscribe can return { success: false, message: '...' } with a 200 status, so a rejected subscription was still busting the statistics cache — causing an unnecessary refetch and a brief stale flicker for unrelated data. Fix: in the mutation cache-invalidation block, derive 'succeeded' from the response body: - no success field present → treat as success (non-envelope endpoints) - success: true → invalidate as before - success: false → skip invalidation entirely Tests added to 'Cache invalidation strategy' suite: • success:false 200 response → cache NOT invalidated • success:true 200 response → cache IS invalidated • no success field present → cache IS invalidated (non-envelope) Closes #1162 --- frontend/src/lib/api/__tests__/client.test.ts | 78 +++++++++++++++++++ frontend/src/lib/api/client.ts | 18 ++++- 2 files changed, 92 insertions(+), 4 deletions(-) diff --git a/frontend/src/lib/api/__tests__/client.test.ts b/frontend/src/lib/api/__tests__/client.test.ts index 5dcf3d6c..a00bf1d7 100644 --- a/frontend/src/lib/api/__tests__/client.test.ts +++ b/frontend/src/lib/api/__tests__/client.test.ts @@ -549,6 +549,84 @@ describe('API Client', () => { const second = await api.getFeaturedMarkets(); expect(second).toEqual([{ id: 1, title: 'Resolved Market' }]); }); + + it('does not invalidate cache when mutation returns success:false (#1162)', async () => { + // Prime a statistics cache entry. + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ total_markets: 10 }), + }); + await api.getStatistics(); + expect(global.fetch).toHaveBeenCalledTimes(1); + + // newsletterSubscribe returns 200 with success:false (business-logic failure). + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ success: false, message: 'Email already subscribed' }), + }); + await api.newsletterSubscribe({ email: 'existing@example.com' }); + expect(global.fetch).toHaveBeenCalledTimes(2); + + // Cache must NOT be invalidated — next getStatistics should return cached value. + const cachedStats = await api.getStatistics(); + expect(cachedStats).toEqual({ total_markets: 10 }); + // fetch should not have been called again (still 2 calls total). + expect(global.fetch).toHaveBeenCalledTimes(2); + }); + + it('invalidates cache when mutation returns success:true (#1162)', async () => { + // Prime a statistics cache entry. + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ total_markets: 10 }), + }); + await api.getStatistics(); + expect(global.fetch).toHaveBeenCalledTimes(1); + + // newsletterSubscribe succeeds. + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ success: true, message: 'Subscribed!' }), + }); + await api.newsletterSubscribe({ email: 'new@example.com' }); + expect(global.fetch).toHaveBeenCalledTimes(2); + + // Cache MUST be invalidated — next getStatistics should hit the network. + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ total_markets: 11 }), + }); + const freshStats = await api.getStatistics(); + expect(freshStats).toEqual({ total_markets: 11 }); + expect(global.fetch).toHaveBeenCalledTimes(3); + }); + + it('invalidates cache for mutations that do not use success envelope (#1162)', async () => { + // Prime a markets cache entry. + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify([{ id: 1, title: 'Market' }]), + }); + await api.getFeaturedMarkets(); + expect(global.fetch).toHaveBeenCalledTimes(1); + + // resolveMarket returns { invalidated_keys: N } (no success field). + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ invalidated_keys: 2 }), + }); + await api.resolveMarket(1); + expect(global.fetch).toHaveBeenCalledTimes(2); + + // Cache MUST be invalidated — next getFeaturedMarkets hits the network. + (global.fetch as jest.Mock).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify([{ id: 1, title: 'Resolved' }]), + }); + const freshMarkets = await api.getFeaturedMarkets(); + expect(freshMarkets).toEqual([{ id: 1, title: 'Resolved' }]); + expect(global.fetch).toHaveBeenCalledTimes(3); + }); }); describe('DELETE requests', () => { diff --git a/frontend/src/lib/api/client.ts b/frontend/src/lib/api/client.ts index 7c904c5c..adc73211 100644 --- a/frontend/src/lib/api/client.ts +++ b/frontend/src/lib/api/client.ts @@ -307,11 +307,21 @@ async function request( // On mutations, invalidate only the affected resource tags instead of // the entire cache. Fall back to a full clear for untagged mutations. + // + // Guard: some endpoints return { success: false, message: '...' } with a + // 200 status to signal business-logic failure (e.g. newsletterSubscribe). + // Only bust the cache when the mutation actually succeeded — i.e. when the + // response has no `success` field (non-envelope endpoints) or when + // `success` is explicitly `true`. if (method === "POST" || method === "DELETE") { - if (options.cacheTags?.length) { - apiCache.invalidateByTags(options.cacheTags); - } else { - apiCache.invalidateByPattern('.*'); + const bodyObj = (typeof data === 'object' && data !== null) ? data as Record : null; + const succeeded = bodyObj === null || !('success' in bodyObj) || bodyObj['success'] === true; + if (succeeded) { + if (options.cacheTags?.length) { + apiCache.invalidateByTags(options.cacheTags); + } else { + apiCache.invalidateByPattern('.*'); + } } }