Skip to content

Commit bf5ceba

Browse files
committed
fix: address critical issues from code review
## Critical Fixes - fix(sdk): add 10s timeout to git operations to prevent hanging - fix(sdk): add maxFiles limit (10000) to discoverProjectFiles to prevent memory issues - fix(billing): add guard for empty paymentMethods array in getOrSetDefaultPaymentMethod - fix(billing): fix card expiration check to use end of month (not start) ## Query Cache Fix - fix(cli): prevent in-flight fetches from resurrecting deleted cache entries - Generation counter now persists after deletion - Added comprehensive tests for deletion protection ## New Tests - test(cli): add unit tests for use-keyboard-navigation hook (40 tests) - test(cli): add unit tests for use-keyboard-shortcuts hook (49 tests) - test(cli): add tests for cache deletion and in-flight request protection (7 tests)
1 parent 4f0ee51 commit bf5ceba

File tree

8 files changed

+2263
-15
lines changed

8 files changed

+2263
-15
lines changed

cli/src/hooks/__tests__/use-activity-query.test.ts

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,13 @@ import {
88
resetActivityQueryCache,
99
isEntryStale,
1010
} from '../use-activity-query'
11+
import {
12+
bumpGeneration,
13+
getGeneration,
14+
deleteCacheEntryCore,
15+
setCacheEntry,
16+
serializeQueryKey,
17+
} from '../../utils/query-cache'
1118

1219
describe('use-activity-query utilities', () => {
1320
beforeEach(() => {
@@ -765,3 +772,193 @@ describe('cache edge cases and error handling', () => {
765772
expect(getActivityQueryData<string>(testKey)).toBe('second')
766773
})
767774
})
775+
776+
/**
777+
* Tests for the cache deletion and in-flight request protection.
778+
* Verifies that in-flight fetches cannot "resurrect" deleted cache entries.
779+
*
780+
* The bug scenario was:
781+
* 1. Fetch starts, captures myGen = 0 (generation not set defaults to 0)
782+
* 2. Entry deleted: bumps generation to 1, then USED TO delete generation entry
783+
* 3. Fetch completes, getGeneration(key) returned 0 again (entry was deleted!)
784+
* 4. 0 === 0 passed, stale fetch wrote to cache, resurrecting the deleted entry
785+
*
786+
* The fix: Don't delete the generation entry after bumping it in deleteCacheEntryCore.
787+
* The bumped generation persists so in-flight requests see a different generation.
788+
*/
789+
describe('cache deletion and in-flight request protection', () => {
790+
beforeEach(() => {
791+
resetActivityQueryCache()
792+
})
793+
794+
test('deletion bumps generation from 0 to 1', () => {
795+
const testKey = ['deletion-gen-test']
796+
const serializedKey = serializeQueryKey(testKey)
797+
798+
// Set initial data
799+
setActivityQueryData(testKey, 'initial-data')
800+
801+
// Before deletion, generation should be 0 (default)
802+
expect(getGeneration(serializedKey)).toBe(0)
803+
804+
// Delete the entry
805+
deleteCacheEntryCore(serializedKey)
806+
807+
// After deletion, generation should be bumped to 1
808+
expect(getGeneration(serializedKey)).toBe(1)
809+
})
810+
811+
test('generation persists after deletion (not cleared)', () => {
812+
const testKey = ['gen-persist-test']
813+
const serializedKey = serializeQueryKey(testKey)
814+
815+
// Set data and delete it
816+
setActivityQueryData(testKey, 'data')
817+
deleteCacheEntryCore(serializedKey)
818+
819+
// Generation should be 1 after first deletion
820+
expect(getGeneration(serializedKey)).toBe(1)
821+
822+
// Data should be gone
823+
expect(getActivityQueryData(testKey)).toBeUndefined()
824+
825+
// Set new data and delete again
826+
setActivityQueryData(testKey, 'new-data')
827+
deleteCacheEntryCore(serializedKey)
828+
829+
// Generation should be 2 after second deletion
830+
expect(getGeneration(serializedKey)).toBe(2)
831+
832+
// This proves generation is NOT being deleted, but accumulated
833+
})
834+
835+
test('simulated in-flight fetch cannot resurrect deleted entry', () => {
836+
const testKey = ['in-flight-protection-test']
837+
const serializedKey = serializeQueryKey(testKey)
838+
839+
// Step 1: Set initial data (simulating a previous successful fetch)
840+
setActivityQueryData(testKey, 'original-data')
841+
expect(getActivityQueryData<string>(testKey)).toBe('original-data')
842+
843+
// Step 2: Simulate a fetch starting - capture the generation
844+
// In real code: const myGen = getGeneration(key)
845+
const myGen = getGeneration(serializedKey)
846+
expect(myGen).toBe(0) // Default generation
847+
848+
// Step 3: While the fetch is "in flight", the entry gets deleted
849+
// (e.g., user navigates away, cache GC runs, or explicit removal)
850+
deleteCacheEntryCore(serializedKey)
851+
852+
// Step 4: Entry is now deleted, but generation was bumped
853+
expect(getActivityQueryData(testKey)).toBeUndefined()
854+
expect(getGeneration(serializedKey)).toBe(1)
855+
856+
// Step 5: The in-flight fetch completes and tries to write
857+
// In real code, this check happens before setCacheEntry:
858+
// if (getGeneration(key) !== myGen) return
859+
const currentGen = getGeneration(serializedKey)
860+
const wouldSkipWrite = currentGen !== myGen
861+
862+
// The write SHOULD be skipped because generations don't match
863+
expect(wouldSkipWrite).toBe(true)
864+
expect(myGen).toBe(0)
865+
expect(currentGen).toBe(1)
866+
867+
// Verify the cache stays empty (entry not resurrected)
868+
expect(getActivityQueryData(testKey)).toBeUndefined()
869+
})
870+
871+
test('generation check correctly allows writes when not deleted', () => {
872+
const testKey = ['gen-allow-write-test']
873+
const serializedKey = serializeQueryKey(testKey)
874+
875+
// Set initial data
876+
setActivityQueryData(testKey, 'initial')
877+
878+
// Capture generation before fetch
879+
const myGen = getGeneration(serializedKey)
880+
expect(myGen).toBe(0)
881+
882+
// Simulate fetch completing WITHOUT any deletion happening
883+
// The generation should still be 0
884+
const currentGen = getGeneration(serializedKey)
885+
const wouldSkipWrite = currentGen !== myGen
886+
887+
// Write should NOT be skipped - generations match
888+
expect(wouldSkipWrite).toBe(false)
889+
890+
// Normal update should work
891+
setActivityQueryData(testKey, 'updated')
892+
expect(getActivityQueryData<string>(testKey)).toBe('updated')
893+
})
894+
895+
test('multiple in-flight fetches all see their respective generations', () => {
896+
const testKey = ['multi-flight-test']
897+
const serializedKey = serializeQueryKey(testKey)
898+
899+
// Fetch 1 starts
900+
const gen1 = getGeneration(serializedKey)
901+
expect(gen1).toBe(0)
902+
903+
// Set some data and delete it
904+
setActivityQueryData(testKey, 'data1')
905+
deleteCacheEntryCore(serializedKey)
906+
907+
// Fetch 2 starts after deletion
908+
const gen2 = getGeneration(serializedKey)
909+
expect(gen2).toBe(1)
910+
911+
// Another deletion
912+
setActivityQueryData(testKey, 'data2')
913+
deleteCacheEntryCore(serializedKey)
914+
915+
// Fetch 3 starts after second deletion
916+
const gen3 = getGeneration(serializedKey)
917+
expect(gen3).toBe(2)
918+
919+
// Now check which fetches would be allowed to write
920+
const currentGen = getGeneration(serializedKey)
921+
expect(currentGen).toBe(2)
922+
923+
// Fetch 1: captured gen 0, current is 2 -> skip write
924+
expect(currentGen !== gen1).toBe(true)
925+
926+
// Fetch 2: captured gen 1, current is 2 -> skip write
927+
expect(currentGen !== gen2).toBe(true)
928+
929+
// Fetch 3: captured gen 2, current is 2 -> allow write
930+
expect(currentGen !== gen3).toBe(false)
931+
})
932+
933+
test('bumpGeneration without deletion also increments generation', () => {
934+
const testKey = ['bump-only-test']
935+
const serializedKey = serializeQueryKey(testKey)
936+
937+
expect(getGeneration(serializedKey)).toBe(0)
938+
939+
bumpGeneration(serializedKey)
940+
expect(getGeneration(serializedKey)).toBe(1)
941+
942+
bumpGeneration(serializedKey)
943+
expect(getGeneration(serializedKey)).toBe(2)
944+
945+
bumpGeneration(serializedKey)
946+
expect(getGeneration(serializedKey)).toBe(3)
947+
})
948+
949+
test('resetActivityQueryCache clears generations', () => {
950+
const testKey = ['reset-gen-test']
951+
const serializedKey = serializeQueryKey(testKey)
952+
953+
// Set data and delete it to bump generation
954+
setActivityQueryData(testKey, 'data')
955+
deleteCacheEntryCore(serializedKey)
956+
expect(getGeneration(serializedKey)).toBe(1)
957+
958+
// Reset cache should clear everything including generations
959+
resetActivityQueryCache()
960+
961+
// Generation should be back to 0 (default)
962+
expect(getGeneration(serializedKey)).toBe(0)
963+
})
964+
})

0 commit comments

Comments
 (0)