From e3fc88b11f42e7463e9040afdf8097488e9d8d96 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 24 Sep 2025 14:37:58 +0200 Subject: [PATCH] fix: cache eviction bug and the eternal loading screen (#10689) Fixes an issue where the project feature list (and potentially other places in the app that use the `useClearSWRCache` hook) would end up in an infinite loading screen because the latest entry that we want to show was overwritten. The primary reason this happened is that we used `keysToDelete = array.slice(SWR_CACHE_SIZE - 1)`. Because the map keys are returned in insertion order, this would make us never delete the oldest keys, but always anything after the cache reached it's maximum size. The fix was to instead do `slice(0, -(SWR_CACHE_SIZE - 1))`, unless that is `0, 0`. If so, then just delete the entire filtered keys set. As a bonus: this PR also deduplicates cache entries that have the same query params but in different order for the feature search. This further reduces the cache space needed. --- .../useFeatureSearch.test.tsx | 19 +++++++++++- .../useFeatureSearch/useFeatureSearch.ts | 5 ++-- frontend/src/hooks/useClearSWRCache.test.ts | 29 +++++++++++++++++-- frontend/src/hooks/useClearSWRCache.ts | 7 ++++- 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.test.tsx b/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.test.tsx index 1f9b229bc5..cd159e13fc 100644 --- a/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.test.tsx +++ b/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.test.tsx @@ -2,7 +2,10 @@ import type { FC } from 'react'; import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; import { testServerRoute, testServerSetup } from 'utils/testServer'; -import { useFeatureSearch } from './useFeatureSearch.ts'; +import { + getFeatureSearchFetcher, + useFeatureSearch, +} from './useFeatureSearch.ts'; import { useSWRConfig } from 'swr'; const server = testServerSetup(); @@ -93,4 +96,18 @@ describe('useFeatureSearch', () => { rerender(); await screen.findByText(/Total: 0/); }); + + test('should give the same cache key, regardless of parameter order', async () => { + const params1 = { + query: 'query-string', + project: 'IS:project', + }; + const params2 = { + project: 'IS:project', + query: 'query-string', + }; + const { KEY: key1 } = getFeatureSearchFetcher(params1); + const { KEY: key2 } = getFeatureSearchFetcher(params2); + expect(key1).toEqual(key2); + }); }); diff --git a/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.ts b/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.ts index 8c45e49612..2d252a5cff 100644 --- a/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.ts +++ b/frontend/src/hooks/api/getters/useFeatureSearch/useFeatureSearch.ts @@ -100,12 +100,13 @@ const createFeatureSearch = () => { export const DEFAULT_PAGE_LIMIT = 25; -const getFeatureSearchFetcher = (params: SearchFeaturesParams) => { +export const getFeatureSearchFetcher = (params: SearchFeaturesParams) => { const urlSearchParams = new URLSearchParams( Array.from( Object.entries(params) .filter(([_, value]) => !!value) - .map(([key, value]) => [key, value.toString()]), // TODO: parsing non-string parameters + .map(([key, value]) => [key, value.toString()]) // TODO: parsing non-string parameters + .toSorted(), ), ).toString(); const KEY = `${PATH}${urlSearchParams}`; diff --git a/frontend/src/hooks/useClearSWRCache.test.ts b/frontend/src/hooks/useClearSWRCache.test.ts index f609ce831f..f6dd61d765 100644 --- a/frontend/src/hooks/useClearSWRCache.test.ts +++ b/frontend/src/hooks/useClearSWRCache.test.ts @@ -14,7 +14,7 @@ describe('manageCacheEntries', () => { expect(cacheMock.has('prefix-3')).toBe(true); }); - it('should keep the SWR_CACHE_SIZE entries and delete the rest', () => { + it('should keep the SWR_CACHE_SIZE entries and delete the rest, keeping the most recent entries', () => { const cacheMock = new Map(); cacheMock.set('prefix-1', {}); cacheMock.set('prefix-2', {}); @@ -23,8 +23,19 @@ describe('manageCacheEntries', () => { clearCacheEntries(cacheMock, 'prefix-4', 'prefix-', 2); - expect(cacheMock.has('prefix-4')).toBe(true); - expect([...cacheMock.keys()].length).toBe(2); + expect([...cacheMock.keys()]).toStrictEqual(['prefix-3', 'prefix-4']); + }); + + it('should not delete the current key, even if it would be cleared as part of the cache size limit', () => { + const cacheMock = new Map(); + cacheMock.set('prefix-1', {}); + cacheMock.set('prefix-2', {}); + cacheMock.set('prefix-3', {}); + cacheMock.set('prefix-4', {}); + + clearCacheEntries(cacheMock, 'prefix-2', 'prefix-', 2); + + expect([...cacheMock.keys()]).toStrictEqual(['prefix-2', 'prefix-4']); }); it('should handle case when SWR_CACHE_SIZE is larger than number of entries', () => { @@ -50,4 +61,16 @@ describe('manageCacheEntries', () => { expect(cacheMock.has('other-2')).toBe(true); expect(cacheMock.has('prefix-3')).toBe(true); }); + + it('treats a negative cache size as zero', () => { + const cacheMock = new Map(); + cacheMock.set('prefix-1', {}); + cacheMock.set('prefix-2', {}); + cacheMock.set('prefix-3', {}); + cacheMock.set('prefix-4', {}); + + clearCacheEntries(cacheMock, 'prefix-3', 'prefix-', -1); + + expect([...cacheMock.keys()]).toStrictEqual(['prefix-3']); + }); }); diff --git a/frontend/src/hooks/useClearSWRCache.ts b/frontend/src/hooks/useClearSWRCache.ts index 32db4c6ce7..50bbdee3e8 100644 --- a/frontend/src/hooks/useClearSWRCache.ts +++ b/frontend/src/hooks/useClearSWRCache.ts @@ -13,7 +13,12 @@ export const clearCacheEntries = ( const filteredKeys = keys.filter( (key) => key.startsWith(clearPrefix) && key !== currentKey, ); - const keysToDelete = filteredKeys.slice(SWR_CACHE_SIZE - 1); + + const entriesToLeave = SWR_CACHE_SIZE - 1; + const keysToDelete = + entriesToLeave <= 0 + ? filteredKeys + : filteredKeys.slice(0, -entriesToLeave); keysToDelete.forEach((key) => cache.delete(key)); };