Conversation
WalkthroughRefactors cacheResponse to a stale-while-revalidate model (drops keepData param), makes MemoryCacheEngine.getFromCache generic (returns T | false), adds cache control helpers, tightens JSON parsing guard in middleware, introduces unit tests for caching and Memoize, and improves test cache isolation and stability. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Cache
participant Helpers as cacheResponse
participant Service as refreshRequest
Note over Caller,Helpers: Stale-while-revalidate (cached & expired)
Caller->>Helpers: cacheResponse(seconds, key, refreshRequest)
Helpers->>Cache: getFromCache(key)
Cache-->>Helpers: { data, ts (expired) }
Helpers-->>Caller: return cached data immediately
alt no pending refresh for key
Helpers->>Service: background refresh (async)
Service-->>Helpers: fresh data
Helpers->>Cache: storeInCache(key, { data, ts: now })
else pending refresh exists
Helpers-->>Helpers: skip duplicate refresh
end
sequenceDiagram
participant Caller
participant Cache
participant Helpers as cacheResponse
participant Service as refreshRequest
Note over Caller,Helpers: Cold miss (no cached data)
Caller->>Helpers: cacheResponse(seconds, key, refreshRequest)
Helpers->>Cache: getFromCache(key)
Cache-->>Helpers: miss (false)
Helpers->>Service: refreshRequest()
Service-->>Helpers: data
Helpers->>Cache: storeInCache(key, { data, ts: now })
Helpers-->>Caller: return fresh data
sequenceDiagram
participant CallerA as Caller A
participant CallerB as Caller B
participant Helpers as cacheResponse
participant Service as refreshRequest
Note over Helpers: Prevent duplicate refreshes
CallerA->>Helpers: cacheResponse(stale cached)
Helpers-->>CallerA: return cached data
Helpers->>Service: start background refresh
CallerB->>Helpers: cacheResponse(stale cached)
Helpers-->>CallerB: return cached data
Helpers-->>Helpers: detect pending refresh, do not start another
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/api/src/caching/helpers.ts (1)
84-97: Consider deduplicating concurrent initial requests.When no cached data exists and multiple concurrent requests arrive for the same key, each will trigger a separate refresh. This differs from the background refresh behavior which properly deduplicates.
Consider checking
pendingRequests[key]before starting a new request:// If no cached data exists, make the request and wait for it logger.debug(`No cached data, making new request for key: ${key}`); + + // If there's already a pending request for this key, wait for it + if (key in pendingRequests) { + logger.debug(`Joining existing request for key: ${key}`); + return (await pendingRequests[key]) as T; + } + pendingRequests[key] = refreshRequest()apps/api/src/provider/controllers/provider/provider.controller.ts (1)
29-31: Unnecessary type casting can be avoided.The double type casting
as unknown as Promise<ProviderListResponse>suggests a type mismatch. SincecacheResponseis generic andgetProviderListalready returns the correct type, the casting shouldn't be necessary.- return cacheResponse(60, scope === "trial" ? cacheKeys.getTrialProviderList : cacheKeys.getProviderList, () => - this.providerService.getProviderList({ trial: scope === "trial" }) - ) as unknown as Promise<ProviderListResponse>; + return cacheResponse<ProviderListResponse>( + 60, + scope === "trial" ? cacheKeys.getTrialProviderList : cacheKeys.getProviderList, + () => this.providerService.getProviderList({ trial: scope === "trial" }) + );apps/api/src/caching/helpers.spec.ts (2)
118-130: Test doesn't verify the actual cache key generation.While the test checks that the decorator creates a function, it doesn't verify that the cache key is correctly generated as
TestClass#testMethod.Consider enhancing the test to verify the actual cache key:
it("should create cache key from class and method name", () => { + const mockCacheResponse = jest.fn(); + jest.doMock("./helpers", () => ({ + ...jest.requireActual("./helpers"), + cacheResponse: mockCacheResponse + })); + class TestClass { @Memoize() async testMethod() { return "test"; } } - const descriptor = Object.getOwnPropertyDescriptor(TestClass.prototype, "testMethod"); - - expect(descriptor?.value).toBeDefined(); - expect(typeof descriptor?.value).toBe("function"); + const instance = new TestClass(); + instance.testMethod(); + + expect(mockCacheResponse).toHaveBeenCalledWith( + 120, + "TestClass#testMethod", + expect.any(Function) + ); });
145-156: Test doesn't verify that string arguments are actually included in the cache key.The test only checks that the function exists but doesn't verify the cache key format with arguments.
Consider testing the actual cache key generation with arguments:
it("should include string arguments in cache key", () => { + const mockCacheResponse = jest.fn(); + jest.doMock("./helpers", () => ({ + ...jest.requireActual("./helpers"), + cacheResponse: mockCacheResponse + })); + class TestClass { @Memoize() async testMethod(_arg1: string, _arg2: string) { return "test"; } } - const descriptor = Object.getOwnPropertyDescriptor(TestClass.prototype, "testMethod"); - - expect(descriptor?.value).toBeDefined(); + const instance = new TestClass(); + instance.testMethod("foo", "bar"); + + expect(mockCacheResponse).toHaveBeenCalledWith( + 120, + "TestClass#testMethod#foo#bar", + expect.any(Function) + ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/caching/helpers.spec.ts(1 hunks)apps/api/src/caching/helpers.ts(1 hunks)apps/api/src/provider/controllers/provider/provider.controller.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/provider/controllers/provider/provider.controller.tsapps/api/src/caching/helpers.spec.tsapps/api/src/caching/helpers.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/provider/controllers/provider/provider.controller.tsapps/api/src/caching/helpers.spec.tsapps/api/src/caching/helpers.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/caching/helpers.spec.ts
🧬 Code Graph Analysis (2)
apps/api/src/provider/controllers/provider/provider.controller.ts (1)
apps/api/src/caching/helpers.ts (2)
cacheResponse(40-100)cacheKeys(102-116)
apps/api/src/caching/helpers.spec.ts (1)
apps/api/src/caching/helpers.ts (3)
cacheEngine(8-8)cacheResponse(40-100)Memoize(22-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (2)
apps/api/src/caching/helpers.ts (2)
36-36: LGTM! Clean API update.The removal of the
keepDataparameter simplifies the API while maintaining backward compatibility through the decorator pattern.
40-51: Solid implementation of stale-while-revalidate pattern.The logic correctly handles the cache existence check and expiration calculation. The pattern of returning stale data immediately while refreshing in the background provides excellent user experience.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1805 +/- ##
==========================================
+ Coverage 42.47% 42.61% +0.14%
==========================================
Files 933 938 +5
Lines 26293 26443 +150
Branches 6979 6927 -52
==========================================
+ Hits 11167 11270 +103
- Misses 14760 14892 +132
+ Partials 366 281 -85
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
apps/api/src/caching/helpers.ts (2)
28-36: Fix key generation edge cases and drop unnecessary await in Memoize.
- filter(Boolean) removes empty string values, causing collisions between "" and omitted args.
- Joining raw strings with "#" can collide with args that themselves contain "#".
- return await is unnecessary here; returning the promise is sufficient.
Apply this diff:
- const argsKey = - args.length > 0 - ? `${cacheKey}#${args - .map(arg => (typeof arg === "string" ? arg : null)) - .filter(Boolean) - .join("#")}` - : cacheKey; - - return await cacheResponse(options?.ttlInSeconds || 60 * 2, argsKey, originalMethod.bind(this, ...args)); + const argsKey = + args.length > 0 + ? `${cacheKey}#${args + .map(arg => (typeof arg === "string" ? encodeURIComponent(arg) : null)) + .filter((arg): arg is string => arg !== null) + .join("#")}` + : cacheKey; + + return cacheResponse(options?.ttlInSeconds || 60 * 2, argsKey, originalMethod.bind(this, ...args));
16-20: Remove unusedkeepDatafromMemoizeOptionsThe
keepDatafield is only declared inapps/api/src/caching/helpers.tsand isn’t referenced anywhere else. To avoid confusion, it should be removed.
- Location:
apps/api/src/caching/helpers.ts:16-20Recommended change:
interface MemoizeOptions { - keepData?: boolean; ttlInSeconds?: number; key?: string; }
🧹 Nitpick comments (1)
apps/api/src/caching/helpers.ts (1)
97-99: Redundant catch that rethrows; let the rejection propagate.The explicit catch that rethrows is unnecessary; finally still executes without it.
Apply this diff:
- .catch(err => { - throw err; - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/caching/helpers.spec.ts(1 hunks)apps/api/src/caching/helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/caching/helpers.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/caching/helpers.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/caching/helpers.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (2)
apps/api/src/caching/helpers.ts (2)
47-51: Confirm intended TTL boundary (strict vs. lenient).Using timeDiff > seconds treats values at exactly the TTL boundary as still fresh. Many TTLs expire at or beyond the threshold, i.e., >=.
If the intended behavior is to expire at the boundary, switch to >=:
- isExpired = timeDiff > seconds; + isExpired = timeDiff >= seconds;Please confirm which semantic you want (strict vs. lenient) and whether tests align with it.
61-79: Background refresh error handling and “no-undefined” cache writes — LGTM.
- Returning cached data on background refresh errors avoids poisoning the cache.
- Guarding writes with data !== undefined prevents overwriting the cache with undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/api/src/caching/helpers.ts (1)
25-35: Prevent cache key collisions in Memoize (drops empty strings; ignores numeric/boolean args).The current key builder:
- Filters out empty strings due to
filter(Boolean), collapsing('', 'foo')into('foo').- Ignores non-string primitives (numbers/booleans), collapsing distinct calls that differ only in these args.
This can return incorrect cached results.Refine the key construction to include string/number/boolean values and preserve empty strings.
Apply this diff:
- const argsKey = - args.length > 0 - ? `${cacheKey}#${args - .map(arg => (typeof arg === "string" ? arg : null)) - .filter(Boolean) - .join("#")}` - : cacheKey; + const argsKey = + args.length > 0 + ? `${cacheKey}#${args + .map(arg => { + const t = typeof arg; + return t === "string" || t === "number" || t === "boolean" ? String(arg) : null; + }) + .filter((v): v is string => v !== null) + .join("#")}` + : cacheKey;
🧹 Nitpick comments (4)
apps/api/src/caching/helpers.ts (4)
16-20: Remove unused keepData from MemoizeOptions.The API dropped keepData; leaving it in the options type is misleading.
Apply this diff:
interface MemoizeOptions { - keepData?: boolean; ttlInSeconds?: number; key?: string; }
36-36: Avoid redundant await in return.No try/catch here;
return awaitjust adds an extra microtask.Apply this diff:
- return await cacheResponse(options?.ttlInSeconds || 60 * 2, argsKey, originalMethod.bind(this, ...args)); + return cacheResponse(options?.ttlInSeconds || 60 * 2, argsKey, originalMethod.bind(this, ...args));
47-51: Simplify expiry calculation to avoid Math.abs.Passing current time as the first arg yields a non-negative delta; no need for
Math.abs.Apply this diff:
- const timeDiff = Math.abs(differenceInSeconds(cachedObject.date, new Date())); + const timeDiff = differenceInSeconds(new Date(), cachedObject.date);
9-9: Prefer Map for pendingRequests for robustness and clarity.Using a plain object risks prototype key collisions (e.g., "proto"), requires
inchecks, and string-only keys. Map better expresses intent.Apply this diff:
-const pendingRequests: { [key: string]: Promise<unknown> } = {}; +const pendingRequests = new Map<string, Promise<unknown>>();And adjust usages:
- if (isExpired && !(key in pendingRequests)) { + if (isExpired && !pendingRequests.has(key)) { logger.debug(`Starting background refresh for key: ${key}`); - pendingRequests[key] = refreshRequest() + pendingRequests.set(key, refreshRequest() .then(data => { logger.debug(`Background refresh completed for key: ${key}`); if (data !== undefined) { cacheEngine.storeInCache(key, { date: new Date(), data: data }); } return data; }) .catch(err => { logger.error({ message: `Error making background cache refresh`, error: err }); return cachedObject.data; }) .finally(() => { - delete pendingRequests[key]; + pendingRequests.delete(key); logger.debug(`Removed pending request for key: ${key}`); - }); + })); }- pendingRequests[key] = - pendingRequests[key] || - refreshRequest() + pendingRequests.set( + key, + pendingRequests.get(key) || + refreshRequest() .then(data => { logger.debug(`New request completed for key: ${key}`); if (data !== undefined) { cacheEngine.storeInCache(key, { date: new Date(), data: data }); } return data; }) .catch(err => { throw err; }) .finally(() => { - delete pendingRequests[key]; + pendingRequests.delete(key); logger.debug(`Removed pending request for key: ${key}`); - }); + }) + ); - return (await pendingRequests[key]) as T; + return (await pendingRequests.get(key)!) as T;Also applies to: 58-80, 90-110
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/caching/helpers.spec.ts(1 hunks)apps/api/src/caching/helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/caching/helpers.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/caching/helpers.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/caching/helpers.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (3)
apps/api/src/caching/helpers.ts (3)
53-84: Stale-while-revalidate path looks solid.
- Returns cached data immediately.
- Starts a guarded background refresh.
- Only overwrites cache on defined data.
- Cleans up pendingRequests in finally.
89-109: Cold-start concurrency guard is correct and prevents duplicate refreshes.Using
pendingRequests[key] = pendingRequests[key] || refreshRequest()ensures only one in-flight request per key. Cleanup in finally is correct; callers await the shared promise.
40-42: Cache engine API is compatible – no changes neededVerified that MemoryCacheEngine.getFromCache returns false on cache misses and that storeInCache(key, data, duration?) accepts two-argument calls (with duration optional). All existing invocations match this signature.
…mance and clarity
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/api/src/caching/helpers.ts (5)
46-50: TTL boundary condition: use >= instead of > for expiryUsing > seconds means entries live one extra second beyond TTL. Recommend >= for conventional TTL semantics.
Apply:
- let isExpired = true; + let isExpired = false; if (hasCachedData) { const timeDiff = differenceInSeconds(new Date(), cachedObject.date); - isExpired = timeDiff > seconds; + isExpired = timeDiff >= seconds; }
90-114: Cold-start: remove redundant catch that just rethrowsThe catch at Line 102 rethrows without adding context or action. It’s redundant and can be dropped to keep the promise chain simpler.
Apply:
- pendingRequest = refreshRequest() + pendingRequest = refreshRequest() .then(data => { logger.debug(`New request completed for key: ${key}`); // Only store in cache if we have valid data if (data !== undefined) { cacheEngine.storeInCache(key, { date: new Date(), data: data }); } return data; }) - .catch(err => { - throw err; - }) .finally(() => { pendingRequests.delete(key); logger.debug(`Removed pending request for key: ${key}`); }); - pendingRequests.set(key, pendingRequest); + pendingRequests.set(key, pendingRequest); } - return (await pendingRequest) as T; + return (await pendingRequest) as T;Additionally, you can slightly improve typing here to avoid the final cast by narrowing the local variable:
- let pendingRequest = pendingRequests.get(key); + let pendingRequest = pendingRequests.get(key) as Promise<T> | undefined;Then the final line can be:
- return (await pendingRequest) as T; + return await pendingRequest;
27-33: Memoize key generation ignores non-string args — potential collisionsOnly string args are considered; calls with different non-string args will collide to the same key. Consider supporting numbers/booleans and a safe serialization for simple objects, or allow a custom argKeyFn in options.
Minimal improvement that avoids JSON-heavy keys but handles common primitives:
- const argsKey = - args.length > 0 - ? `${cacheKey}#${args - .map(arg => (typeof arg === "string" ? arg : null)) - .filter(Boolean) - .join("#")}` - : cacheKey; + const argsKey = + args.length > 0 + ? `${cacheKey}#${args + .map(arg => { + if (typeof arg === "string" || typeof arg === "number" || typeof arg === "boolean") return String(arg); + return null; + }) + .filter(Boolean) + .join("#")}` + : cacheKey;If you want full control, extend MemoizeOptions with an argKeyFn: (args: unknown[]) => string and use it when provided.
40-44: Avoid the cast by making cache engine genericThe getFromCache result is cast to CachedObject | false. If MemoryCacheEngine.getFromCache is changed to be generic, this cast becomes unnecessary.
Proposed change in memoryCacheEngine.ts (not this file):
// before getFromCache(key: string): unknown | false // after getFromCache<T>(key: string): { date: Date; data: T } | falseThen here you can write:
const cachedObject = cacheEngine.getFromCache<T>(key);
9-9: pendingRequests typing is fine; minor ergonomic improvement availableKeeping Map<string, Promise> at module scope is appropriate since T varies per key. Locally casting the retrieved promise to Promise (as shown in the cold-path comment) helps avoid the final return cast and keeps types tighter within the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/caching/helpers.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/caching/helpers.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/caching/helpers.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
🔇 Additional comments (3)
apps/api/src/caching/helpers.ts (3)
56-81: Stale-while-revalidate path and in-flight dedupe look solidReturns stale data immediately, dedupes refreshes with pendingRequests, avoids clobbering cache with undefined, and cleans up in finally. Good work.
65-67: Confirm storing null vs undefined semanticsYou guard against undefined before storing; null will be stored. Is null intended as a valid “no data” marker for callers? If not, also guard against null to avoid overwriting cache with an empty payload.
If null should not be stored, apply:
- if (data !== undefined) { + if (data !== undefined && data !== null) { cacheEngine.storeInCache(key, { date: new Date(), data: data }); }And similarly in the cold path store.
Also applies to: 97-99
39-41: Breaking API change addressed: cacheResponse call sites updated– No instances of
cacheResponsewith four arguments found
– No references to the removedkeepDataparameter detectedAll call sites now use the new three-parameter signature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/api/test/setup-functional-tests.ts (2)
10-10: Global cache cleanup before setup and after teardown looks correct.This avoids test bleed-through across suites. Minor improvement: consider wrapping teardown in try/finally to guarantee cache clearing even if teardown throws.
Apply this diff to make cleanup resilient:
afterAll(async () => { - await dbService.teardown(); - cacheEngine.clearAllKeyInCache(); + try { + await dbService.teardown(); + } finally { + cacheEngine.clearAllKeyInCache(); + } }, 10000);Also applies to: 16-16
19-21: Per-test clear in both beforeEach and afterEach is redundant.Clearing in afterEach is sufficient (it runs even when tests fail), and avoids duplicate work. If you prefer belt-and-suspenders, keep as-is; otherwise, drop one to reduce overhead.
Here’s a minimal change retaining afterEach only:
-beforeEach(() => { - cacheEngine.clearAllKeyInCache(); -}); - afterEach(() => { cacheEngine.clearAllKeyInCache(); });Also applies to: 23-25
apps/api/src/caching/memoryCacheEngine.ts (2)
8-8: Consider returning T | undefined instead of using a boolean sentinel.A boolean sentinel complicates typing and prevents caching boolean false distinctly. Returning undefined on miss is idiomatic and plays well with optional chaining and nullish coalescing. This would require small call-site adjustments.
Proposed signature:
- getFromCache<T>(key: string): T | false { + getFromCache<T>(key: string): T | undefined { const cachedBody = mcache.get(key); - if (cachedBody !== null && cachedBody !== undefined) { - return cachedBody as T; - } - return false; + return cachedBody === null || cachedBody === undefined ? undefined : (cachedBody as T); }If you want, I can follow through with a codemod to update call sites that rely on
=== false.
4-13: Tighten JSDoc to reflect actual types and behavior.Docs say “memcache” and use {*} which isn’t helpful. Align comments with the typed signature and nullish behavior.
/** - * Used to retrieve data from memcache - * @param {*} key + * Retrieve a cached value. + * @param {string} key Cache key + * @returns {T|false} Cached value when present, otherwise false */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/caching/helpers.ts(2 hunks)apps/api/src/caching/memoryCacheEngine.ts(1 hunks)apps/api/test/setup-functional-tests.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/caching/helpers.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/test/setup-functional-tests.tsapps/api/src/caching/memoryCacheEngine.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/test/setup-functional-tests.tsapps/api/src/caching/memoryCacheEngine.ts
🧠 Learnings (2)
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/api/test/setup-functional-tests.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns it
Applied to files:
apps/api/test/setup-functional-tests.ts
🧬 Code Graph Analysis (1)
apps/api/test/setup-functional-tests.ts (1)
apps/api/src/caching/helpers.ts (1)
cacheEngine(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (1)
apps/api/test/setup-functional-tests.ts (1)
3-3: Good call centralizing cache control via the singleton.Importing cacheEngine from helpers ensures a single in-process cache is cleared consistently across tests.
| const cachedBody = mcache.get(key); | ||
| if (cachedBody) { | ||
| return cachedBody; | ||
| return cachedBody as T; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix falsy-value bug in cache hit detection.
Using a truthiness check causes valid cached values like 0, "" or false to be treated as a cache miss. Use a nullish check instead.
Apply this diff:
getFromCache<T>(key: string): T | false {
const cachedBody = mcache.get(key);
- if (cachedBody) {
+ if (cachedBody !== null && cachedBody !== undefined) {
return cachedBody as T;
}
return false;
}To confirm no callers intentionally store falsy primitives (which would still be fine after this fix, but worth validating expectations), run:
🏁 Script executed:
#!/bin/bash
# Find storeInCache calls and flag obviously falsy literals
rg -n --no-heading $'storeInCache\\s*\\(' -A 2 | sed -n '1,200p'
# Spot calls explicitly passing false/0/""/null as data
rg -n --no-heading $'storeInCache\\s*\\([^,]+,\\s*(false|0|""|\'\'|null)\\b' -A 0Length of output: 2071
Fix falsy-value bug in cache hit detection
This change ensures that valid cached values such as 0, "", or false aren’t treated as cache misses. We’ve checked all storeInCache calls and found no cases where primitives like false, 0, "", or null are intentionally stored, so this update is safe.
Apply this diff in apps/api/src/caching/memoryCacheEngine.ts:
getFromCache<T>(key: string): T | false {
const cachedBody = mcache.get(key);
- if (cachedBody) {
+ if (cachedBody !== null && cachedBody !== undefined) {
return cachedBody as T;
}
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cachedBody = mcache.get(key); | |
| if (cachedBody) { | |
| return cachedBody; | |
| return cachedBody as T; | |
| } | |
| return false; | |
| const cachedBody = mcache.get(key); | |
| if (cachedBody !== null && cachedBody !== undefined) { | |
| return cachedBody as T; | |
| } | |
| return false; |
🤖 Prompt for AI Agents
In apps/api/src/caching/memoryCacheEngine.ts around lines 9 to 13, the current
cache hit test uses a truthy check (if (cachedBody)) which treats valid cached
values like 0, "", or false as misses; change the condition to an explicit
undefined check (e.g., if (cachedBody !== undefined)) so any stored value
(including falsy primitives) is returned, otherwise return false to indicate a
miss.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/api/src/middlewares/userMiddleware.ts (1)
8-20: Fix falsy-miss bug, avoid double cache read, and improve typing
- Using if (!result) treats "", 0, and false as cache misses. Prefer a nullish check (result == null) to only treat undefined/null as misses.
- Avoid the second cache read; return the already-fetched result.
- Tighten types to avoid implicit any and make the API clearer. Also annotate put’s return type.
Apply this diff:
- async get(key: string, format: string) { - const result = cacheEngine.getFromCache(key); - if (!result) { - return null; - } else if (format === "json" && typeof result === "string") { - return JSON.parse(result); - } else { - return cacheEngine.getFromCache(key); - } + async get<T = unknown>(key: string, format?: "json"): Promise<T | null> { + const result: unknown = cacheEngine.getFromCache(key); + if (result == null) { + return null; + } else if (format === "json" && typeof result === "string") { + try { + return JSON.parse(result) as T; + } catch { + // Treat malformed JSON as a cache miss to allow fresh fetch + return null; + } + } else { + return result as T; + } }, - async put(key: string, value: unknown) { + async put(key: string, value: unknown): Promise<void> { cacheEngine.storeInCache(key, value); }Notes:
- If the narrower signature (format?: "json") or the generic return type causes type incompatibilities with verifyRsaJwt’s kvStore shape, keep format: string but still return Promise<unknown | null> and retain the nullish check/double-read fix:
- async get(key: string, format: string) { + async get(key: string, format: string): Promise<unknown | null> { - const result = cacheEngine.getFromCache(key); + const result: unknown = cacheEngine.getFromCache(key); - if (!result) { + if (result == null) { return null; } else if (format === "json" && typeof result === "string") { - return JSON.parse(result); + try { return JSON.parse(result) as unknown; } catch { return null; } } else { - return cacheEngine.getFromCache(key); + return result; } }
🧹 Nitpick comments (1)
apps/api/src/middlewares/userMiddleware.ts (1)
25-25: Use property shorthand for kvStoreMinor readability nit: use object property shorthand.
- kvStore: kvStore + kvStore- kvStore: kvStore, + kvStore,Also applies to: 30-30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/middlewares/userMiddleware.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/middlewares/userMiddleware.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/middlewares/userMiddleware.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (1)
apps/api/src/middlewares/userMiddleware.ts (1)
12-13: Safer JSON.parse guard — good changeAdding typeof result === "string" before JSON.parse prevents runtime errors when non-string values are cached. This aligns with the broader caching refactor.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/test/functional/provider-graph-data.spec.ts (1)
291-293: Move timer restoration into a finally blockRestoring timers only at the end of the happy path can leak timers on failure. Co-locate this with the setup in a try/finally as suggested above.
🧹 Nitpick comments (1)
apps/api/test/functional/market-data.spec.ts (1)
12-15: Persisted nock interceptors: add net-connect guard and ensure cleanupPersisting interceptors is fine for repeated calls, and you clean them later. To further harden tests:
- Disable real network connections to fail fast on missing mocks.
- Optionally re-enable in afterAll if other suites rely on network.
Suggested additions:
beforeAll(async () => { // Clean up any existing nock interceptors nock.cleanAll(); + // Prevent real HTTP calls during this suite + nock.disableNetConnect(); const coinGeckoApiUrl = "https://api.coingecko.com"; // More permissive nock setup that matches any headers nock(coinGeckoApiUrl) .persist()And in afterAll:
afterAll(() => { nock.cleanAll(); + nock.enableNetConnect(); });Also applies to: 31-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/api/test/functional/lease-flow.spec.ts(5 hunks)apps/api/test/functional/market-data.spec.ts(3 hunks)apps/api/test/functional/provider-graph-data.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/test/functional/provider-graph-data.spec.tsapps/api/test/functional/market-data.spec.tsapps/api/test/functional/lease-flow.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/test/functional/provider-graph-data.spec.tsapps/api/test/functional/market-data.spec.tsapps/api/test/functional/lease-flow.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/test/functional/provider-graph-data.spec.tsapps/api/test/functional/market-data.spec.tsapps/api/test/functional/lease-flow.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/test/functional/provider-graph-data.spec.ts
🧬 Code Graph Analysis (1)
apps/api/test/functional/lease-flow.spec.ts (1)
apps/api/src/caching/helpers.ts (1)
cacheEngine(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (4)
apps/api/test/functional/lease-flow.spec.ts (2)
74-74: Allowance increase looks correctUpdating deploymentAllowance to 10_000_000 aligns with the subsequent deposit flow and reduces the risk of insufficiency during the test.
158-160: No need toawaitcacheEngine.clearAllKeyInCache (it's synchronous)After inspecting MemoryCacheEngine.clearAllKeyInCache(), it simply calls mcache.clear() and does not return a Promise. Awaiting it has no effect, so the current calls at lines 158–160, 217–219, and 261–263 in lease-flow.spec.ts (and the similar calls in setup-functional-tests.ts and helpers.spec.ts) are correct as-is. If you ever introduce an async cache-clearing implementation in the future, you can revisit and add
await.apps/api/test/functional/market-data.spec.ts (2)
7-9: Good isolation: cleaning interceptors before tests startResetting nock interceptors up-front prevents carry-over from previous suites and helps ensure deterministic runs.
48-50: Solid teardownCleaning interceptors after the suite avoids cross-suite interference.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/api/test/functional/lease-flow.spec.ts (3)
19-19: Good: decoupled test cache clearing via helperImporting
clearCachefrom@test/setup-functional-testsremoves the direct dependency on the cache engine and aligns with previous feedback. This improves maintainability and test isolation.
217-219: Same as above: use wildcard or a shared constant for cache key/prefixMake the prefix intent explicit and avoid magic strings.
- clearCache("WalletController#getBalances"); + clearCache("WalletController#getBalances*");
261-263: Same as above: use wildcard or a shared constant for cache key/prefixRepeat the explicit prefix clearing here for consistency.
- clearCache("WalletController#getBalances"); + clearCache("WalletController#getBalances*");
🧹 Nitpick comments (1)
apps/api/test/functional/lease-flow.spec.ts (1)
158-160: Prefer explicit prefix clearing (use wildcard) or extract a named constant
clearCache("WalletController#getBalances")relies on the helper’s heuristic to decide between key vs prefix clearing. Make it explicit and less brittle by using a wildcard to guarantee prefix invalidation, or extract a constant to avoid repeating a magic string.Apply this change here:
- clearCache("WalletController#getBalances"); + clearCache("WalletController#getBalances*");Optionally, define a constant near the imports and reuse in all three places to reduce duplication:
// near imports const BALANCES_CACHE_PREFIX = "WalletController#getBalances*";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/api/src/caching/memoryCacheEngine.ts(2 hunks)apps/api/test/functional/deployment-setting.spec.ts(1 hunks)apps/api/test/functional/lease-flow.spec.ts(5 hunks)apps/api/test/functional/provider-graph-data.spec.ts(1 hunks)apps/api/test/setup-functional-tests.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/api/test/functional/deployment-setting.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/caching/memoryCacheEngine.ts
- apps/api/test/functional/provider-graph-data.spec.ts
- apps/api/test/setup-functional-tests.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/test/functional/lease-flow.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/test/functional/lease-flow.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/test/functional/lease-flow.spec.ts
🧬 Code Graph Analysis (1)
apps/api/test/functional/lease-flow.spec.ts (1)
apps/api/test/setup-functional-tests.ts (1)
clearCache(13-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
🔇 Additional comments (1)
apps/api/test/functional/lease-flow.spec.ts (1)
74-74: Verify increased deploymentAllowance value and unitsBumping
deploymentAllowanceto 10_000_000 provides ample headroom, but please confirm the unit matches the deposit inputs used in this flow (e.g., AKT vs uAKT) so we’re not masking potential unit mismatches or regressions. If feasible, consider deriving this from a shared constant to avoid divergence with production defaults.
* fix: improve caching function * fix: return cache data if error * fix: improve concurrency check * refactor: replace pendingRequests object with Map for improved performance and clarity * refactor: enhance type safety in caching functions and improve cache expiration logic * fix: ensure JSON parsing only occurs for string results in cache retrieval * test: enhance lease flow tests with cache clearing and update deployment allowance * feat: add cache management functions and enhance test cache clearing logic
2 participants
#1803
Summary by CodeRabbit
Refactor
Bug Fixes
Tests