fix: clean up cache by removing stale keys #252
Conversation
|
@sbansal1999 is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughfetchAllFlags now builds batch cache entries with computed ttl/staleTime, prunes in-memory cache keys not present in the latest API results via a new helper, and writes the prepared entries to the cache instead of performing inline per-entry updates. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (9)**/*.{ts,tsx,js,jsx,vue}📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Files:
**/*.{ts,tsx,js,jsx,css,scss,sass,less}📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Files:
**/*.{ts,tsx,jsx}📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Files:
!(**/pages/_document.{ts,tsx,jsx})**/*.{ts,tsx,jsx}📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Files:
**/*.{ts,tsx,html,css}📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)
Files:
**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)
Files:
🧬 Code graph analysis (1)packages/sdk/src/core/flags/flags-manager.ts (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a cache consistency issue by ensuring stale flag keys are removed from the in-memory cache when Key changes:
Impact: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant FlagsManager
participant API
participant Cache
participant Storage
Client->>FlagsManager: fetchAllFlags()
FlagsManager->>API: fetchAllFlagsApi(apiUrl, params)
API-->>FlagsManager: flags (Record<string, FlagResult>)
Note over FlagsManager: Build cache entries array
FlagsManager->>FlagsManager: Map flags to {cacheKey, cacheEntry}[]
Note over FlagsManager: Extract valid cache keys
FlagsManager->>FlagsManager: validCacheKeys = new Set(cacheKeys)
FlagsManager->>Cache: removeStaleKeys(validCacheKeys)
Note over Cache: Delete keys not in validCacheKeys
loop For each cached key
alt key not in validCacheKeys
Cache->>Cache: delete(key)
end
end
Note over FlagsManager: Populate cache with new entries
loop For each flag entry
FlagsManager->>Cache: set(cacheKey, cacheEntry)
end
FlagsManager->>Storage: saveToStorage()
Note over Storage: Storage.setAll() also removes stale keys
Storage->>Storage: Compare currentKeys vs newKeys
Storage->>Storage: deleteMultiple(removedKeys)
FlagsManager->>Client: notifyUpdate()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
packages/sdk/src/core/flags/flags-manager.ts, line 305 (link)logic: logic error: cache keys have user suffix but flags object has plain keys. mismatch causes all user-specific cache entries to be deleted incorrectly. need to extract base flag key with
key.split(":")[0]before checking against flags object
1 file reviewed, 1 comment
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/sdk/src/core/flags/flags-manager.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,vue}: Never block paste in<input>or<textarea>elements
Enter submits focused text input; in<textarea>, ⌘/Ctrl+Enter submits; Enter adds newline
Compatible with password managers and 2FA; allow pasting one-time codes
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Trim values to handle text expansion and trailing spaces in form submissions
URL reflects state (deep-link filters/tabs/pagination/expanded panels); prefer libraries likenuqs
Back/Forward buttons restore scroll position
Delay first tooltip in a group; subsequent peers have no delay
Use locale-aware formatting for dates, times, numbers, and currency
Batch layout reads/writes; avoid unnecessary reflows/repaints
Virtualize large lists using libraries likevirtua
**/*.{ts,tsx,js,jsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include...
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,js,jsx,css,scss,sass,less}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,css,scss,sass,less}: During drag operations, disable text selection and setinerton dragged element and containers
Animations must be interruptible and input-driven; avoid autoplay
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,jsx}: Use semantic elements instead of role attributes in JSX.
Don't use unnecessary fragments.
Don't pass children as props.
Don't use the return value of React.render.
Make sure all dependencies are correctly specified in React hooks.
Make sure all React hooks are called from the top level of component functions.
Don't forget key props in iterators and collection literals.
Don't destructure props inside JSX components in Solid projects.
Don't define React components inside other components.
Don't use event handlers on non-interactive elements.
Don't assign to React component props.
Don't use bothchildrenanddangerouslySetInnerHTMLprops on the same element.
Don't use dangerous JSX props.
Don't use Array index in keys.
Don't insert comments as text nodes.
Don't assign JSX properties multiple times.
Don't add extra closing tags for components without children.
Use<>...</>instead of<Fragment>...</Fragment>.
Watch out for possible "wrong" semicolons inside JSX elements.
Make sure void (self-closing) elements don't have children.
Don't usetarget="_blank"withoutrel="noopener".
Don't use<img>elements in Next.js projects.
Don't use<head>elements in Next.js projects.
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Do NOT use types 'any', 'unknown' or 'never'. Use proper explicit types
Suffix functions with 'Action' in types, like 'type Test = { testAction }'
Files:
packages/sdk/src/core/flags/flags-manager.ts
!(**/pages/_document.{ts,tsx,jsx})**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx in Next.js projects.
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,html,css}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor bundling
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)
**/*.{js,jsx,ts,tsx}: Split off components, utils, and reusable code to ensure better loading speed and less complexity
Use lower-case-like-this naming convention for variables, functions, and identifiers
NEVER add placeholders, mock data, or anything similar to production code
Use Dayjs for date handling, NEVER use date-fns. Use Tanstack query for hooks, NEVER use SWR
Use json.stringify() when adding debugging code
Never use barrel exports or create index files
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)
Handle complex data transformations independently of React. Keep modules decoupled from React for improved modularity and testability
Files:
packages/sdk/src/core/flags/flags-manager.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). (1)
- GitHub Check: Greptile Review
| const existingKeys = Array.from(this.cache.keys()); | ||
| const missingKeys = existingKeys.filter((key) => !flags[key]); | ||
| for (const key of missingKeys) { | ||
| this.cache.delete(key); | ||
| } |
There was a problem hiding this comment.
Critical: Cache key format mismatch causes all entries to be deleted.
The cache keys include user context (format: "flagKey:userContext"), but the flags object uses plain flag keys. Line 305 compares the full cache key against flags[key], which will always be undefined, causing all cache entries to be incorrectly deleted immediately after fetching.
Additionally, this logic doesn't account for multiple user contexts. When fetching flags for one user, it should only clean up stale entries for that specific user, not delete cached flags for other users.
🔎 Proposed fix
const existingKeys = Array.from(this.cache.keys());
-const missingKeys = existingKeys.filter((key) => !flags[key]);
+const currentUser = user ?? this.config.user;
+const currentUserKeys = existingKeys.filter((key) => {
+ const cacheKey = getCacheKey("", currentUser);
+ return key.endsWith(cacheKey.slice(1)); // Match user suffix
+});
+const missingKeys = currentUserKeys.filter((key) => {
+ const flagKey = key.split(":")[0];
+ return !flags[flagKey];
+});
for (const key of missingKeys) {
this.cache.delete(key);
}Or, more simply if getCacheKey format is predictable:
const existingKeys = Array.from(this.cache.keys());
-const missingKeys = existingKeys.filter((key) => !flags[key]);
+const missingKeys = existingKeys.filter((key) => {
+ const flagKey = key.split(":")[0];
+ return !flags[flagKey];
+});
for (const key of missingKeys) {
this.cache.delete(key);
}Note: The simpler version still has the multi-user issue. Consider whether cleaning up other users' cache entries is intentional.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/sdk/src/core/flags/flags-manager.ts around lines 304 to 308, the
cleanup is comparing the full cache key (format "flagKey:userContext") to plain
flag keys, so the condition always fails and deletes every cache entry; fix by
extracting the flag key and user context from each cache key (e.g., split on ':'
using the known getCacheKey format), then only delete entries where the
extracted user context matches the current fetch's userContext and the extracted
flag key does not exist in the fetched flags object; ensure you use the same
key-formatting logic as getCacheKey and avoid touching cache entries belonging
to other users.
sbansal1999
force-pushed
the
flags-improv
branch
from
a0d78d2 to
8215de7
Compare
|
@greptile review again chief |
There was a problem hiding this comment.
Additional Comments (1)
-
packages/sdk/src/core/flags/flags-manager.ts, line 304-309 (link)logic: cache map uses composite keys (flag name plus user context, e.g.,
featureX:123:[email protected]) butflagsobject keys are just flag names (e.g.,featureX). this mismatch means!flags[key]will always be true for user-contextualized cache entries, causing all entries to be deleted instead of only stale ones. extract the flag name from cache keys before comparison:cacheKey.split(":")[0]
1 file reviewed, 1 comment
sbansal1999
force-pushed
the
flags-improv
branch
from
8215de7 to
ef8caa0
Compare
|
@greptile go crazy |
There was a problem hiding this comment.
Looking at this cache cleanup implementation, I can see some opportunities for optimization and robustness! Here are several improvements we could make:
Performance Optimizations:
- Batch operations: The current approach iterates twice - once to build cache entries, once to set them. We could combine these.
- Reduce allocations: Creating a Set and multiple objects could be optimized.
Robustness Improvements:
3. Error handling: What if cache operations fail? Should we rollback or continue?
4. Atomic updates: Currently there's a window where cache is partially cleared but not fully updated.
5. Memory efficiency: For large flag sets, we're holding extra data structures.
Alternative Implementation:
private async initialize(): Promise<void> {
// Load from persistent storage first (instant hydration)
if (!this.config.skipStorage && this.storage) {
Wait, let me look at the actual lines that need changing. The comment is on the fetchAllFlags method around lines 302-320. Here's a more efficient approach:
const apiUrl = this.config.apiUrl ?? "https://api.databuddy.cc";
const params = buildQueryParams(this.config, user);
const ttl = this.config.cacheTtl ?? 60_000;
const staleTime = this.config.staleTime ?? ttl / 2;
try {
const flags = await fetchAllFlagsApi(apiUrl, params);
// Collect new cache keys while building entries
const newCacheKeys = new Set<string>();
const cacheUpdates = new Map<string, CacheEntry<FlagResult>>();
for (const [key, result] of Object.entries(flags)) {
const cacheKey = getCacheKey(key, user ?? this.config.user);
newCacheKeys.add(cacheKey);
cacheUpdates.set(cacheKey, createCacheEntry(result, ttl, staleTime));
}
// Remove stale keys in one pass
for (const existingKey of this.cache.keys()) {
if (!newCacheKeys.has(existingKey)) {
this.cache.delete(existingKey);
}
}
// Apply all updates atomically
for (const [cacheKey, cacheEntry] of cacheUpdates) {
this.cache.set(cacheKey, cacheEntry);
}
this.ready = true;
This approach:
- Eliminates the extra array allocation from
map() - Removes the separate
removeStaleKeyscall - Groups all cache mutations together for better atomicity
- Uses a single loop for deletion instead of Set lookup overhead
Want to make it even more aggressive? We could add cache versioning, LRU eviction, or background cleanup! 🚀
|
@greptile review this PR |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/sdk/src/core/flags/flags-manager.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,vue}: Never block paste in<input>or<textarea>elements
Enter submits focused text input; in<textarea>, ⌘/Ctrl+Enter submits; Enter adds newline
Compatible with password managers and 2FA; allow pasting one-time codes
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Trim values to handle text expansion and trailing spaces in form submissions
URL reflects state (deep-link filters/tabs/pagination/expanded panels); prefer libraries likenuqs
Back/Forward buttons restore scroll position
Delay first tooltip in a group; subsequent peers have no delay
Use locale-aware formatting for dates, times, numbers, and currency
Batch layout reads/writes; avoid unnecessary reflows/repaints
Virtualize large lists using libraries likevirtua
**/*.{ts,tsx,js,jsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include...
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,js,jsx,css,scss,sass,less}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,css,scss,sass,less}: During drag operations, disable text selection and setinerton dragged element and containers
Animations must be interruptible and input-driven; avoid autoplay
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,jsx}: Use semantic elements instead of role attributes in JSX.
Don't use unnecessary fragments.
Don't pass children as props.
Don't use the return value of React.render.
Make sure all dependencies are correctly specified in React hooks.
Make sure all React hooks are called from the top level of component functions.
Don't forget key props in iterators and collection literals.
Don't destructure props inside JSX components in Solid projects.
Don't define React components inside other components.
Don't use event handlers on non-interactive elements.
Don't assign to React component props.
Don't use bothchildrenanddangerouslySetInnerHTMLprops on the same element.
Don't use dangerous JSX props.
Don't use Array index in keys.
Don't insert comments as text nodes.
Don't assign JSX properties multiple times.
Don't add extra closing tags for components without children.
Use<>...</>instead of<Fragment>...</Fragment>.
Watch out for possible "wrong" semicolons inside JSX elements.
Make sure void (self-closing) elements don't have children.
Don't usetarget="_blank"withoutrel="noopener".
Don't use<img>elements in Next.js projects.
Don't use<head>elements in Next.js projects.
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Do NOT use types 'any', 'unknown' or 'never'. Use proper explicit types
Suffix functions with 'Action' in types, like 'type Test = { testAction }'
Files:
packages/sdk/src/core/flags/flags-manager.ts
!(**/pages/_document.{ts,tsx,jsx})**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx in Next.js projects.
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{ts,tsx,html,css}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor bundling
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)
**/*.{js,jsx,ts,tsx}: Split off components, utils, and reusable code to ensure better loading speed and less complexity
Use lower-case-like-this naming convention for variables, functions, and identifiers
NEVER add placeholders, mock data, or anything similar to production code
Use Dayjs for date handling, NEVER use date-fns. Use Tanstack query for hooks, NEVER use SWR
Use json.stringify() when adding debugging code
Never use barrel exports or create index files
Files:
packages/sdk/src/core/flags/flags-manager.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)
Handle complex data transformations independently of React. Keep modules decoupled from React for improved modularity and testability
Files:
packages/sdk/src/core/flags/flags-manager.ts
| private removeStaleKeys(validCacheKeys: Set<string>): void { | ||
| for (const key of this.cache.keys()) { | ||
| if (!validCacheKeys.has(key)) { | ||
| this.cache.delete(key); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical: Multi-user cache corruption.
The method deletes all cache keys not present in validCacheKeys, but when called from fetchAllFlags (line 315), validCacheKeys only contains keys for the current user. If the cache holds entries for multiple users, fetching flags for User A will incorrectly delete all cached flags for User B.
Scenario:
- User A fetches → cache:
"flag1:userA","flag2:userA" - User B fetches → cache:
"flag1:userA","flag2:userA","flag1:userB","flag2:userB" - User A re-fetches →
validCacheKeys={"flag1:userA", "flag2:userA"} - All User B entries are deleted because they're not in
validCacheKeys
🔎 Proposed fix to scope cleanup to current user
-private removeStaleKeys(validCacheKeys: Set<string>): void {
- for (const key of this.cache.keys()) {
- if (!validCacheKeys.has(key)) {
- this.cache.delete(key);
+private removeStaleKeys(validCacheKeys: Set<string>, currentUser?: UserContext): void {
+ const userSuffix = getCacheKey("", currentUser ?? this.config.user).slice(1); // Get ":userContext" part
+ for (const key of this.cache.keys()) {
+ // Only consider keys belonging to the current user
+ if (key.endsWith(userSuffix) && !validCacheKeys.has(key)) {
+ this.cache.delete(key);
}
}
}Then update the call site at line 315:
this.removeStaleKeys(
- new Set(flagCacheEntries.map(({ cacheKey }) => cacheKey))
+ new Set(flagCacheEntries.map(({ cacheKey }) => cacheKey)),
+ user
);Committable suggestion skipped: line range outside the PR's diff.
| const ttl = this.config.cacheTtl ?? 60_000; | ||
| const staleTime = this.config.staleTime ?? ttl / 2; | ||
|
|
||
| try { | ||
| const flags = await fetchAllFlagsApi(apiUrl, params); | ||
| const flagCacheEntries = Object.entries(flags).map(([key, result]) => ({ | ||
| cacheKey: getCacheKey(key, user ?? this.config.user), | ||
| cacheEntry: createCacheEntry(result, ttl, staleTime), | ||
| })); | ||
|
|
||
| // Update cache with all flags | ||
| const ttl = this.config.cacheTtl ?? 60_000; | ||
| const staleTime = this.config.staleTime ?? ttl / 2; | ||
| this.removeStaleKeys( | ||
| new Set(flagCacheEntries.map(({ cacheKey }) => cacheKey)) | ||
| ); | ||
|
|
||
| for (const [key, result] of Object.entries(flags)) { | ||
| const cacheKey = getCacheKey(key, user ?? this.config.user); | ||
| this.cache.set(cacheKey, createCacheEntry(result, ttl, staleTime)); | ||
| for (const { cacheKey, cacheEntry } of flagCacheEntries) { | ||
| this.cache.set(cacheKey, cacheEntry); | ||
| } |
There was a problem hiding this comment.
Partially fixes the original issue but introduces multi-user bug.
What's fixed:
- Lines 310-313 correctly construct cache keys using
getCacheKey(key, user ?? this.config.user), which fixes the cache key format mismatch from the previous review. - The batch preparation approach (lines 310-313, 319-321) is cleaner and more efficient.
New issue:
- Line 315-317 calls
removeStaleKeyswith only the current user's cache keys, but the method deletes entries for ALL users not in that set. See the critical issue flagged in theremoveStaleKeysmethod above.
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
sbansal1999
force-pushed
the
flags-improv
branch
from
ef8caa0 to
d1f140e
Compare
2 participants
Description
The change ensures that any keys in the cache that are no longer present in the latest
flagsobject are removed, preventing stale or outdated cache entries from persisting.Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.