From ead89168bd859bec68049781380d029bb1d2085d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=88=E5=AE=B6=E5=90=8D?= Date: Wed, 25 Mar 2026 20:42:12 +0800 Subject: [PATCH] Normalize activity query cache keys --- .../__tests__/use-activity-query.test.ts | 96 ++++++++++--------- .../__tests__/use-claude-quota-query.test.ts | 48 +++++----- cli/src/hooks/use-activity-query.ts | 56 +++++++---- 3 files changed, 117 insertions(+), 83 deletions(-) diff --git a/cli/src/hooks/__tests__/use-activity-query.test.ts b/cli/src/hooks/__tests__/use-activity-query.test.ts index 12ceea8657..b93e0bc23e 100644 --- a/cli/src/hooks/__tests__/use-activity-query.test.ts +++ b/cli/src/hooks/__tests__/use-activity-query.test.ts @@ -131,6 +131,14 @@ describe('use-activity-query utilities', () => { ) }) + test('object key property order does not create duplicate cache entries', () => { + setActivityQueryData(['query', { page: 1, sort: 'asc' }], 'page1') + + expect( + getActivityQueryData(['query', { sort: 'asc', page: 1 }]), + ).toBe('page1') + }) + test('nested objects in keys work correctly', () => { setActivityQueryData( ['query', { filter: { status: 'active', type: 'user' } }], @@ -143,6 +151,20 @@ describe('use-activity-query utilities', () => { ]), ).toBe('filtered') }) + + test('nested object property order does not create duplicate cache entries', () => { + setActivityQueryData( + ['query', { page: 1, filter: { status: 'active', type: 'user' } }], + 'filtered', + ) + + expect( + getActivityQueryData([ + 'query', + { filter: { type: 'user', status: 'active' }, page: 1 }, + ]), + ).toBe('filtered') + }) }) }) @@ -419,7 +441,6 @@ describe('polling and staleness simulation', () => { test('data becomes stale after staleTime passes', () => { const testKey = ['stale-time-test'] - const serializedKey = JSON.stringify(testKey) const staleTime = 30000 // 30 seconds // Set data at t=0 @@ -427,16 +448,16 @@ describe('polling and staleness simulation', () => { // Data was set at mockNow (1000000), so dataUpdatedAt = 1000000 expect(getActivityQueryData(testKey)).toBe('fresh-data') - expect(isEntryStale(serializedKey, staleTime)).toBe(false) // Fresh + expect(isEntryStale(testKey, staleTime)).toBe(false) // Fresh // Advance time by 25 seconds - still fresh mockNow += 25000 - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(testKey, staleTime)).toBe(false) // Advance time past staleTime mockNow += 10000 // Now 35 seconds have passed // Data should now be considered stale (35s > 30s staleTime) - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(testKey, staleTime)).toBe(true) // The data is still accessible even when stale expect(getActivityQueryData(testKey)).toBe('fresh-data') @@ -444,16 +465,15 @@ describe('polling and staleness simulation', () => { test('invalidated data is immediately stale', () => { const testKey = ['invalidate-stale-test'] - const serializedKey = JSON.stringify(testKey) const staleTime = 30000 // Set fresh data setActivityQueryData(testKey, 'data') - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(testKey, staleTime)).toBe(false) // Invalidate immediately makes it stale (dataUpdatedAt = 0) invalidateActivityQuery(testKey) - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(testKey, staleTime)).toBe(true) // Data still exists but would be refetched on next access expect(getActivityQueryData(testKey)).toBe('data') @@ -461,46 +481,43 @@ describe('polling and staleness simulation', () => { test('updating data resets the staleness timer', () => { const testKey = ['reset-timer-test'] - const serializedKey = JSON.stringify(testKey) const staleTime = 30000 // Set initial data setActivityQueryData(testKey, 'initial') - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(testKey, staleTime)).toBe(false) // Advance time past staleTime mockNow += 35000 - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(testKey, staleTime)).toBe(true) // Update data - should reset the timer setActivityQueryData(testKey, 'updated') - expect(isEntryStale(serializedKey, staleTime)).toBe(false) // Fresh again + expect(isEntryStale(testKey, staleTime)).toBe(false) // Fresh again // Data is fresh again expect(getActivityQueryData(testKey)).toBe('updated') // Advance a little bit - should still be fresh mockNow += 10000 - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(testKey, staleTime)).toBe(false) expect(getActivityQueryData(testKey)).toBe('updated') }) test('staleTime of 0 means always stale', () => { const testKey = ['zero-stale-test'] - const serializedKey = JSON.stringify(testKey) // Set data setActivityQueryData(testKey, 'data') // With staleTime=0, data is always considered stale // (this means refetch should happen on every interval tick) - expect(isEntryStale(serializedKey, 0)).toBe(true) + expect(isEntryStale(testKey, 0)).toBe(true) expect(getActivityQueryData(testKey)).toBe('data') }) test('non-existent key is always stale', () => { - const serializedKey = JSON.stringify(['non-existent']) - expect(isEntryStale(serializedKey, 30000)).toBe(true) + expect(isEntryStale(['non-existent'], 30000)).toBe(true) }) }) @@ -624,7 +641,7 @@ describe('Claude subscription update scenarios', () => { mockNow += 35000 // 35 seconds // Data is now stale, polling tick should trigger refetch - // In real code: if (isEntryStale(serializedKey, staleTime)) void doFetch() + // In real code: if (isEntryStale(testKey, staleTime)) void doFetch() // Simulate what refetch would do const newQuota = { fiveHourRemaining: 75, sevenDayRemaining: 95 } @@ -809,7 +826,6 @@ describe('error-only entries and persistent error handling', () => { // 5. This prevents immediate refetch loop const testKey = ['error-only-fresh-test'] - const serializedKey = JSON.stringify(testKey) const staleTime = 30000 // 30 seconds const error = new Error('API error') @@ -819,12 +835,11 @@ describe('error-only entries and persistent error handling', () => { // Entry has errorUpdatedAt = 1000000, current time = 1000000 // Time since error: 0ms, staleTime: 30000ms // Should NOT be stale because error is recent - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(testKey, staleTime)).toBe(false) }) test('error-only entry becomes stale after staleTime passes', () => { const testKey = ['error-stale-after-time-test'] - const serializedKey = JSON.stringify(testKey) const staleTime = 30000 // 30 seconds const error = new Error('API error') @@ -832,15 +847,15 @@ describe('error-only entries and persistent error handling', () => { setErrorOnlyCacheEntry(testKey, error, mockNow) // Initially not stale - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(testKey, staleTime)).toBe(false) // Advance time by 25 seconds - still fresh mockNow += 25000 - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(testKey, staleTime)).toBe(false) // Advance time past staleTime (now 35 seconds since error) mockNow += 10000 - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(testKey, staleTime)).toBe(true) }) test('simulates subscription query polling with persistent errors', () => { @@ -851,7 +866,6 @@ describe('error-only entries and persistent error handling', () => { // - With fix: isEntryStale uses errorUpdatedAt, preventing rapid refetches const subscriptionKey = ['subscription', 'current'] - const serializedKey = JSON.stringify(subscriptionKey) const staleTime = 30000 // 30 seconds (matches useSubscriptionQuery) const refetchInterval = 60000 // 60 seconds const error = new Error('Failed to fetch subscription: 500') @@ -861,75 +875,72 @@ describe('error-only entries and persistent error handling', () => { // Immediately after error, entry should NOT be stale // This is the critical fix - prevents immediate refetch loop - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(subscriptionKey, staleTime)).toBe(false) // Simulate polling interval at t=1s (as reported in bug) mockNow += 1000 // Entry should still NOT be stale (only 1s since error, staleTime is 30s) - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(subscriptionKey, staleTime)).toBe(false) // Simulate many 1-second intervals - none should trigger refetch until staleTime for (let i = 0; i < 28; i++) { mockNow += 1000 - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(subscriptionKey, staleTime)).toBe(false) } // Now at t=29s - should still be fresh (29s is not > 30s) - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(subscriptionKey, staleTime)).toBe(false) // At t=30s - should still be fresh (30s is not > 30s, need strictly greater) mockNow += 1000 - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(isEntryStale(subscriptionKey, staleTime)).toBe(false) // At t=31s - now stale, refetch should be allowed (31s > 30s) mockNow += 1000 - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(subscriptionKey, staleTime)).toBe(true) }) test('staleTime of 0 means always stale even for error-only entries', () => { const testKey = ['zero-stale-error-test'] - const serializedKey = JSON.stringify(testKey) const error = new Error('Some error') setErrorOnlyCacheEntry(testKey, error, mockNow) - + // With staleTime=0, entry is always considered stale - expect(isEntryStale(serializedKey, 0)).toBe(true) + expect(isEntryStale(testKey, 0)).toBe(true) }) test('error-only entry with null errorUpdatedAt is stale', () => { // Edge case: if somehow errorUpdatedAt is null, entry should be stale // This shouldn't happen in practice but tests defensive coding const testKey = ['null-error-time-test'] - const serializedKey = JSON.stringify(testKey) const staleTime = 30000 // Create entry without errorUpdatedAt (using undefined which gets stored as null) // Note: setErrorOnlyCacheEntry always sets errorUpdatedAt, so we test via regular data // and then invalidate it - + // Non-existent key is stale - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(testKey, staleTime)).toBe(true) }) test('successful data takes precedence over errorUpdatedAt for staleness', () => { const testKey = ['data-precedence-test'] - const serializedKey = JSON.stringify(testKey) const staleTime = 30000 // First, set an error-only entry setErrorOnlyCacheEntry(testKey, new Error('Initial error'), mockNow) - expect(isEntryStale(serializedKey, staleTime)).toBe(false) // Fresh error + expect(isEntryStale(testKey, staleTime)).toBe(false) // Fresh error // Now set successful data (this is what happens on successful retry) setActivityQueryData(testKey, { subscription: 'active' }) - + // Staleness should now be based on dataUpdatedAt, not errorUpdatedAt - expect(isEntryStale(serializedKey, staleTime)).toBe(false) // Fresh data + expect(isEntryStale(testKey, staleTime)).toBe(false) // Fresh data // Advance time past staleTime mockNow += 35000 - expect(isEntryStale(serializedKey, staleTime)).toBe(true) // Stale based on dataUpdatedAt + expect(isEntryStale(testKey, staleTime)).toBe(true) // Stale based on dataUpdatedAt }) }) @@ -1105,10 +1116,9 @@ describe('retry infinite loop bug fix (subscription 401 scenario)', () => { // Error entry should exist (data is undefined but entry exists) // The entry has error set, which we can verify via isEntryStale behavior - const serializedKey = JSON.stringify(queryKey) // Entry exists (not stale due to "no entry" - stale due to other reasons) // Since we just set errorUpdatedAt = Date.now(), it should not be stale // for a reasonable staleTime - expect(isEntryStale(serializedKey, 30000)).toBe(false) + expect(isEntryStale(queryKey, 30000)).toBe(false) }) }) diff --git a/cli/src/hooks/__tests__/use-claude-quota-query.test.ts b/cli/src/hooks/__tests__/use-claude-quota-query.test.ts index 1f1913c374..6a29d67ab1 100644 --- a/cli/src/hooks/__tests__/use-claude-quota-query.test.ts +++ b/cli/src/hooks/__tests__/use-claude-quota-query.test.ts @@ -594,7 +594,7 @@ describe('Polling and cache freshness', () => { test('data should become stale after staleTime (30s)', () => { const staleTime = 30000 // 30 seconds - const serializedKey = JSON.stringify(claudeQuotaQueryKeys.current()) + const queryKey = claudeQuotaQueryKeys.current() // Set quota data at t=0 const quota: ClaudeQuotaData = { @@ -603,18 +603,18 @@ describe('Polling and cache freshness', () => { sevenDayRemaining: 60, sevenDayResetsAt: null, } - setActivityQueryData(claudeQuotaQueryKeys.current(), quota) + setActivityQueryData(queryKey, quota) // At this point, dataUpdatedAt = mockNow (1000000) - expect(getActivityQueryData(claudeQuotaQueryKeys.current())).toBeDefined() - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + expect(getActivityQueryData(queryKey)).toBeDefined() + expect(isEntryStale(queryKey, staleTime)).toBe(false) // Advance time by 35 seconds (past staleTime) mockNow += 35000 // Data is stale but still accessible - expect(isEntryStale(serializedKey, staleTime)).toBe(true) - const cached = getActivityQueryData(claudeQuotaQueryKeys.current()) + expect(isEntryStale(queryKey, staleTime)).toBe(true) + const cached = getActivityQueryData(queryKey) expect(cached?.fiveHourRemaining).toBe(50) // In the actual hook, this would trigger a refetch on the next interval tick @@ -622,46 +622,46 @@ describe('Polling and cache freshness', () => { test('refreshed data should reset staleness', () => { const staleTime = 30000 - const serializedKey = JSON.stringify(claudeQuotaQueryKeys.current()) + const queryKey = claudeQuotaQueryKeys.current() // Set initial data - setActivityQueryData(claudeQuotaQueryKeys.current(), { fiveHourRemaining: 100 }) - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + setActivityQueryData(queryKey, { fiveHourRemaining: 100 }) + expect(isEntryStale(queryKey, staleTime)).toBe(false) // Advance past staleTime mockNow += 35000 - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(queryKey, staleTime)).toBe(true) // "Refetch" by setting new data - setActivityQueryData(claudeQuotaQueryKeys.current(), { fiveHourRemaining: 80 }) - expect(isEntryStale(serializedKey, staleTime)).toBe(false) // Fresh again + setActivityQueryData(queryKey, { fiveHourRemaining: 80 }) + expect(isEntryStale(queryKey, staleTime)).toBe(false) // Fresh again // Data is now fresh expect( - getActivityQueryData<{ fiveHourRemaining: number }>(claudeQuotaQueryKeys.current())?.fiveHourRemaining, + getActivityQueryData<{ fiveHourRemaining: number }>(queryKey)?.fiveHourRemaining, ).toBe(80) // Advance a little (less than staleTime) mockNow += 10000 - expect(isEntryStale(serializedKey, staleTime)).toBe(false) // Still fresh + expect(isEntryStale(queryKey, staleTime)).toBe(false) // Still fresh }) test('invalidation should mark data for immediate refetch', () => { const staleTime = 30000 - const serializedKey = JSON.stringify(claudeQuotaQueryKeys.current()) + const queryKey = claudeQuotaQueryKeys.current() // Set data - setActivityQueryData(claudeQuotaQueryKeys.current(), { fiveHourRemaining: 70 }) - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + setActivityQueryData(queryKey, { fiveHourRemaining: 70 }) + expect(isEntryStale(queryKey, staleTime)).toBe(false) // Invalidate (sets dataUpdatedAt to 0) - invalidateActivityQuery(claudeQuotaQueryKeys.current()) - expect(isEntryStale(serializedKey, staleTime)).toBe(true) // Immediately stale + invalidateActivityQuery(queryKey) + expect(isEntryStale(queryKey, staleTime)).toBe(true) // Immediately stale // Data exists but is immediately stale (dataUpdatedAt === 0) // Next poll interval will trigger refetch regardless of time elapsed expect( - getActivityQueryData<{ fiveHourRemaining: number }>(claudeQuotaQueryKeys.current())?.fiveHourRemaining, + getActivityQueryData<{ fiveHourRemaining: number }>(queryKey)?.fiveHourRemaining, ).toBe(70) }) @@ -672,15 +672,15 @@ describe('Polling and cache freshness', () => { const staleTime = 30 * 1000 // useClaudeQuotaQuery config const refetchInterval = 60 * 1000 // chat.tsx config - const serializedKey = JSON.stringify(claudeQuotaQueryKeys.current()) + const queryKey = claudeQuotaQueryKeys.current() // Initial fetch - setActivityQueryData(claudeQuotaQueryKeys.current(), { fiveHourRemaining: 100 }) - expect(isEntryStale(serializedKey, staleTime)).toBe(false) + setActivityQueryData(queryKey, { fiveHourRemaining: 100 }) + expect(isEntryStale(queryKey, staleTime)).toBe(false) // After 60 seconds (when refetch interval fires), data should be stale mockNow += refetchInterval - expect(isEntryStale(serializedKey, staleTime)).toBe(true) + expect(isEntryStale(queryKey, staleTime)).toBe(true) // This confirms that the refetch interval tick WILL trigger a new fetch // because the data is stale at that point (60s > 30s staleTime) diff --git a/cli/src/hooks/use-activity-query.ts b/cli/src/hooks/use-activity-query.ts index 971a9942a5..5daaae7e23 100644 --- a/cli/src/hooks/use-activity-query.ts +++ b/cli/src/hooks/use-activity-query.ts @@ -47,6 +47,8 @@ const snapshotMemo = new Map< } >() +type ActivityQueryKey = readonly unknown[] + /** * Notify listeners for a specific cache key. */ @@ -111,8 +113,8 @@ function getCacheEntry(key: string): CacheEntry | undefined { * Check if a cache entry is stale based on staleTime. * Exported for testing purposes. */ -export function isEntryStale(key: string, staleTime: number): boolean { - const entry = getCacheEntry(key) +export function isEntryStale(queryKey: ActivityQueryKey | string, staleTime: number): boolean { + const entry = getCacheEntry(resolveQueryCacheKey(queryKey)) if (!entry) return true // If we have successful data, use its timestamp for staleness @@ -162,8 +164,36 @@ function getRefCount(key: string): number { /** * Serialize a query key to a string for cache lookup. */ -function serializeQueryKey(queryKey: readonly unknown[]): string { - return JSON.stringify(queryKey) +function normalizeQueryKeyPart(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map((item) => normalizeQueryKeyPart(item)) + } + + if (value instanceof Date) { + return value.toJSON() + } + + if (value instanceof URL) { + return value.toString() + } + + if (value && typeof value === 'object') { + return Object.fromEntries( + Object.entries(value as Record) + .sort(([left], [right]) => left.localeCompare(right)) + .map(([key, entryValue]) => [key, normalizeQueryKeyPart(entryValue)]), + ) + } + + return value +} + +function serializeQueryKey(queryKey: ActivityQueryKey): string { + return JSON.stringify(normalizeQueryKeyPart(queryKey)) +} + +function resolveQueryCacheKey(queryKey: ActivityQueryKey | string): string { + return typeof queryKey === 'string' ? queryKey : serializeQueryKey(queryKey) } // Module-level map to track GC timeouts (survives component unmount) @@ -205,7 +235,7 @@ function deleteCacheEntry(key: string): void { } export type UseActivityQueryOptions = { /** Unique key for caching the query */ - queryKey: readonly unknown[] + queryKey: ActivityQueryKey /** Function that fetches the data */ queryFn: () => Promise /** Whether the query is enabled (default: true) */ @@ -270,7 +300,6 @@ export function useActivityQuery( } = options const serializedKey = serializeQueryKey(queryKey) - const mountedRef = useRef(true) const intervalRef = useRef | null>(null) const wasIdleRef = useRef(false) @@ -408,7 +437,6 @@ export function useActivityQuery( // Initial fetch on mount/key change/enabled toggle (intentionally minimal deps) useEffect(() => { - mountedRef.current = true if (!enabled) return const currentEntry = getCacheEntry(serializedKey) @@ -423,10 +451,6 @@ export function useActivityQuery( (!currentEntry) if (shouldFetchOnMount) void doFetch() - - return () => { - mountedRef.current = false - } }, [enabled, serializedKey]) // Polling @@ -510,7 +534,7 @@ export function useActivityQuery( /** * Invalidate a query, causing it to refetch on next access. */ -export function invalidateActivityQuery(queryKey: readonly unknown[]): void { +export function invalidateActivityQuery(queryKey: ActivityQueryKey): void { const key = serializeQueryKey(queryKey) const entry = getCacheEntry(key) if (!entry) return @@ -520,7 +544,7 @@ export function invalidateActivityQuery(queryKey: readonly unknown[]): void { /** * Remove a query from the cache entirely. */ -export function removeActivityQuery(queryKey: readonly unknown[]): void { +export function removeActivityQuery(queryKey: ActivityQueryKey): void { const key = serializeQueryKey(queryKey) const existingTimeout = gcTimeouts.get(key) @@ -535,7 +559,7 @@ export function removeActivityQuery(queryKey: readonly unknown[]): void { /** * Read cached data. */ -export function getActivityQueryData(queryKey: readonly unknown[]): T | undefined { +export function getActivityQueryData(queryKey: ActivityQueryKey): T | undefined { const key = serializeQueryKey(queryKey) return getCacheEntry(key)?.data } @@ -543,7 +567,7 @@ export function getActivityQueryData(queryKey: readonly unknown[]): T | undef /** * Write cached data (optimistic updates). */ -export function setActivityQueryData(queryKey: readonly unknown[], data: T): void { +export function setActivityQueryData(queryKey: ActivityQueryKey, data: T): void { const key = serializeQueryKey(queryKey) setCacheEntry(key, { data, @@ -585,7 +609,7 @@ export function resetActivityQueryCache(): void { * This simulates what happens when a fetch fails with no prior successful data. */ export function setErrorOnlyCacheEntry( - queryKey: readonly unknown[], + queryKey: ActivityQueryKey, error: Error, errorUpdatedAt?: number, ): void {