Add ability to clear cache and individual keys and new policy that allows cache size management in MB #9
Conversation
|
HELLO??? |
|
Hi @phazei , sorry to lately reply. I will take a look and process it ASAP. Tks for your contribution |
WalkthroughThis update introduces new cache management and eviction features, including a size-based LFU policy, explicit cache clearing and removal methods, and enhanced synchronization between memory and file system caches. Several interfaces and classes are extended with methods for clearing caches and removing entries. Minor dependency and configuration updates are also included. Changes
Sequence Diagram(s)Video Cache Removal FlowsequenceDiagram
participant App
participant useAsyncCache
participant cacheManager
participant MemoryCache
participant FileSystem
App->>useAsyncCache: removeVideoFromCache(url)
useAsyncCache->>cacheManager: removeCachedVideo(url)
cacheManager->>MemoryCache: syncCache(url)
cacheManager->>FileSystem: remove file (if exists)
useAsyncCache->>useAsyncCache: clear local state if url matches
Memory Cache Eviction with Size PolicysequenceDiagram
participant MemoryCacheProvider
participant LFUSizePolicy
participant FileSystemManager
MemoryCacheProvider->>LFUSizePolicy: onEvict(cache, delegate, triggerKey)
LFUSizePolicy->>FileSystemManager: getStatisticList(cacheDir)
LFUSizePolicy->>MemoryCacheProvider: remove least frequently used entries
LFUSizePolicy->>FileSystemManager: remove files as needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds cache management capabilities and introduces a new size-based cache policy. It fixes several build issues and improves cache reliability by handling stale cache entries.
- Added new cache management methods to clear cache and remove individual cache entries
- Introduced LFUSizePolicy for managing cache size in MB instead of number of entries
- Fixed stale cache handling where cached keys existed but files were missing from filesystem
Reviewed Changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ProxyCacheManager.ts | Added cache management methods and stale cache cleanup logic |
| src/Provider/MemoryCacheLFUSizePolicy.ts | New cache policy that manages cache by size in MB |
| src/Provider/MemoryCacheProvider.ts | Enhanced with clear method and eviction improvements |
| src/Provider/MemoryCacheLFUPolicy.ts | Fixed fallback eviction when all items have equal frequency |
| src/Hooks/useCache.ts | Added removeVideoFromCache hook for component-level cache management |
| package.json | Updated React Native version from 0.72.6 to 0.72.17 |
| ); | ||
|
|
||
| let totalSize = files.reduce( | ||
| (sum, file) => sum + parseInt(file.size as unknown as string, 10), |
There was a problem hiding this comment.
Using parseInt on file.size with type casting as unknown as string is unsafe. If file.size is already a number, this will return NaN. Consider checking the type first or using Number(file.size) instead.
| (sum, file) => sum + parseInt(file.size as unknown as string, 10), | |
| (sum, file) => sum + Number(file.size), |
Copilot uses AI. Check for mistakes.
| // console.log('::::::::::::::::: REFERENCE_BIT', this.referenceBit); | ||
| // console.log('::::::::::::::::: CACHE', Object.fromEntries(cache)); |
There was a problem hiding this comment.
Remove commented-out console.log statements. These debug logs should not be committed to production code.
| // console.log('::::::::::::::::: REFERENCE_BIT', this.referenceBit); | |
| // console.log('::::::::::::::::: CACHE', Object.fromEntries(cache)); | |
Copilot uses AI. Check for mistakes.
| // console.log('::::::::::::: COUNT', count, ':::'); | ||
| // console.log('::::::::::::: EVICTKEY', count, evictedKey, ':::'); | ||
| // console.log('::::::::::::: FILES', count, files.length, ':::'); |
There was a problem hiding this comment.
Remove commented-out console.log statements. These debug logs should not be committed to production code.
| // console.log('::::::::::::: COUNT', count, ':::'); | |
| // console.log('::::::::::::: EVICTKEY', count, evictedKey, ':::'); | |
| // console.log('::::::::::::: FILES', count, files.length, ':::'); | |
Copilot uses AI. Check for mistakes.
|
|
||
| /** | ||
| * | ||
| - LFUSize (Least Recently Used by Size): The least recently used item is evicted. This bases the eviction check on cache directory size in MB. |
There was a problem hiding this comment.
The comment incorrectly describes LFUSize as 'Least Recently Used by Size' when it should be 'Least Frequently Used by Size' based on the class name and implementation.
Copilot uses AI. Check for mistakes.
| : this.referenceBit[key]! + 1; | ||
| } | ||
|
|
||
| async onEvict( |
There was a problem hiding this comment.
The onEvict method is called on every cache access (line 35 in MemoryCacheProvider.ts), but it performs expensive file system operations including directory scanning and file size calculations. Consider implementing a more efficient approach like periodic cleanup or size tracking.
Copilot uses AI. Check for mistakes.
| this.cachePolicy.onEvict(this.cache, this.delegate, key); | ||
|
|
There was a problem hiding this comment.
Calling onEvict on every cache get operation could significantly impact performance, especially for the LFUSizePolicy which performs file system operations. Consider calling onEvict only when necessary (e.g., when cache is near capacity).
| this.cachePolicy.onEvict(this.cache, this.delegate, key); |
Copilot uses AI. Check for mistakes.
| return cachedKey; | ||
| } else { | ||
| // File missing - clean up cache entries | ||
| this._memoryCache?.syncCache(url); |
There was a problem hiding this comment.
[nitpick] The method name 'syncCache' is unclear. Based on the context where it's used to clean up stale cache entries, a more descriptive name like 'removeEntry' or 'cleanupStaleEntry' would be better.
| this._memoryCache?.syncCache(url); | |
| this._memoryCache?.cleanupStaleEntry(url); |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/Libs/fileSystem.ts (1)
136-142: Add method binding in constructor for consistency.The new
getStatisticListmethod should be bound in the constructor like other methods (line 32 showsgetStatisticbeing bound).Add this line in the constructor after line 32:
this.getStatistic.bind(this); + this.getStatisticList.bind(this);src/ProxyCacheManager.ts (2)
153-157: Simplify redundant null checkThe explicit if check is redundant since optional chaining already handles the null/undefined case safely.
clearMemoryCache(): void { - if (this._memoryCache) { - this._memoryCache?.clear(); - } + this._memoryCache?.clear(); }
168-191: Consider logging file deletion errorsWhile the logic correctly prioritizes memory cache cleanup over file deletion success, silently swallowing errors could make debugging difficult in production.
try { await this.didEvictHandler(key, cachedPath); } catch (error) { // Still succeeded in cleaning cache/policy even if file deletion failed + console.warn(`Failed to delete cached file for ${key}:`, error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
example/package.json(1 hunks)package.json(1 hunks)react-native-cache-video.podspec(1 hunks)src/Hooks/useCache.ts(1 hunks)src/Libs/fileSystem.ts(1 hunks)src/Provider/MemoryCacheFreePolicy.ts(1 hunks)src/Provider/MemoryCacheLFUPolicy.ts(4 hunks)src/Provider/MemoryCacheLFUSizePolicy.ts(1 hunks)src/Provider/MemoryCacheProvider.ts(4 hunks)src/Provider/PreCacheProvider.ts(0 hunks)src/Provider/index.ts(1 hunks)src/ProxyCacheManager.ts(3 hunks)src/types/type.d.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/Provider/PreCacheProvider.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Provider/MemoryCacheLFUPolicy.ts (1)
src/Utils/constants.ts (1)
SECOND_CHANCE_TO_COUNT(5-5)
src/ProxyCacheManager.ts (2)
src/Utils/util.ts (1)
getCacheKey(188-196)src/Utils/constants.ts (1)
KEY_PREFIX(6-6)
🔇 Additional comments (19)
example/package.json (1)
14-14: LGTM! Routine patch version update.The React Native patch version update from 0.72.6 to 0.72.17 aligns with the main package.json and should include bug fixes and improvements.
package.json (1)
69-69: LGTM! Consistent dependency update.The React Native patch version update maintains consistency with the example package and should provide bug fixes and improvements.
react-native-cache-video.podspec (1)
14-14: iOS deployment target aligns with React Native 0.72.17
- react-native-cache-video.podspec (line 14):
s.platforms = { :ios => "12.4" }matches the minimum iOS version specified in nearly all React Native 0.72.17 podspecs.- No further code changes are required.
Please ensure you document this bump to iOS 12.4 as a breaking change in your release notes or README so users on older iOS versions are aware.
src/Provider/index.ts (1)
2-2: LGTM! Clean export addition for new cache policy.The export addition properly exposes the new
MemoryCacheLFUSizePolicyclass and follows the existing export pattern.src/Provider/MemoryCacheFreePolicy.ts (1)
10-14: LGTM! Clean implementation of interface requirements.The empty stub methods for
clear()andremoveEntry()are appropriate for the FreePolicy since it doesn't maintain any internal state or tracking data. The simplified constructor is also a good cleanup.src/Hooks/useCache.ts (1)
62-79: Well-implemented cache removal function.The
removeVideoFromCachefunction properly integrates with the cache manager and handles local state cleanup when the removed video matches the currently cached video. The memoization and dependency management are correct.src/Provider/MemoryCacheLFUPolicy.ts (3)
34-40: LGTM! Clean state management methods.The
clear()andremoveEntry()methods properly manage the internalreferenceBittracking state.
89-99: Smart fallback eviction strategy.The fallback to evict the oldest item when no suitable frequency-based candidate is found ensures the cache capacity constraint is always maintained.
68-82: Excellent improvements to eviction logic.The stale key cleanup prevents memory leaks, and the enhanced frequency comparison properly handles the
SECOND_CHANCE_TO_COUNTspecial case. However, there's a subtle issue with the eviction logic.The condition on line 77
freq !== SECOND_CHANCE_TO_COUNT || lfuKey === nullmay not work as intended. IflfuKeyis already set to a non-SECOND_CHANCE_TO_COUNT item, this condition could still replace it with a SECOND_CHANCE_TO_COUNT item when a better candidate exists.Consider this fix:
- if (freq && freq < minFreq) { - // Consider SECOND_CHANCE_TO_COUNT items if nothing else found - if (freq !== SECOND_CHANCE_TO_COUNT || lfuKey === null) { - minFreq = freq; - lfuKey = key; - } - } + if (freq && freq < minFreq && freq !== SECOND_CHANCE_TO_COUNT) { + minFreq = freq; + lfuKey = key; + } else if (freq === SECOND_CHANCE_TO_COUNT && lfuKey === null) { + // Only use SECOND_CHANCE_TO_COUNT items as last resort + minFreq = freq; + lfuKey = key; + }Likely an incorrect or invalid review comment.
src/types/type.d.ts (2)
52-58: Well-designed interface enhancements.The updated
onEvictsignature with optionaltriggerKeyparameter and the newclear()andremoveEntry()methods provide the necessary contracts for enhanced cache management while maintaining backward compatibility.
72-72: Good addition to cache interface.The
clear()method addition toMemoryCacheInterfaceprovides a consistent API for cache clearing operations.src/Provider/MemoryCacheProvider.ts (4)
35-35: Good integration with enhanced eviction logic.Passing the current key to
onEvictenables cache policies to make more informed eviction decisions.
49-49: Consistent eviction integration.The
putmethod properly passes the trigger key to enable enhanced eviction logic.
62-64: Essential cleanup integration.Calling
cachePolicy.removeEntry(key)during cache removal prevents memory leaks by cleaning up policy tracking data.
104-110: Well-coordinated cache clearing.The
clear()method properly clears both the internal cache map and the policy's tracking data, ensuring complete cache reset.src/ProxyCacheManager.ts (3)
46-58: Good simplification of the constructorRemoving explicit
.bind(this)calls is appropriate since class methods in TypeScript are automatically bound to the instance context.
106-117: Excellent fix for stale cache entriesThis enhancement properly handles the case where cache keys exist in memory but the corresponding files are missing on the filesystem, maintaining consistency between the two layers.
159-166: Well-implemented cache clearing logicThe method correctly clears both memory cache and disk files in the proper order, maintaining consistency across both storage layers.
src/Provider/MemoryCacheLFUSizePolicy.ts (1)
42-55: Well-implemented hybrid LFU/LRU access trackingThe method effectively combines LFU frequency tracking with LRU ordering by repositioning accessed items in the Map. The use of
SECOND_CHANCE_TO_COUNTfor new entries is a good design choice.
| constructor(capacityMB: number) { | ||
| this.referenceBit = {} as { | ||
| [key in string]: number; | ||
| }; | ||
| this.capacityBytes = capacityMB * 1024 * 1024; // Convert MB to bytes | ||
| this.storage = new FileSystemManager(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for capacity parameter and consider FileSystemManager injection
The constructor should validate the capacityMB parameter and consider dependency injection for FileSystemManager.
-constructor(capacityMB: number) {
+constructor(capacityMB: number, storage?: FileSystemManager) {
+ if (capacityMB <= 0) {
+ throw new Error('Capacity must be greater than 0 MB');
+ }
this.referenceBit = {} as {
[key in string]: number;
};
this.capacityBytes = capacityMB * 1024 * 1024; // Convert MB to bytes
- this.storage = new FileSystemManager();
+ this.storage = storage || new FileSystemManager();
}🤖 Prompt for AI Agents
In src/Provider/MemoryCacheLFUSizePolicy.ts around lines 26 to 32, add
validation to ensure the capacityMB parameter is a positive number and throw an
error if it is invalid. Modify the constructor to accept an optional
FileSystemManager instance as a parameter for dependency injection, defaulting
to a new instance if none is provided. This improves robustness and testability.
| async onEvict( | ||
| cache: Map<string, any>, | ||
| delegate?: MemoryCacheDelegate<any>, | ||
| triggerKey?: string | ||
| ) { | ||
| if (this.isEvicting) { | ||
| return; // Another eviction is in progress | ||
| } | ||
| try { | ||
| this.isEvicting = true; | ||
|
|
||
| // Get current directory size | ||
| const files = await this.storage.getStatisticList( | ||
| this.storage.getBucketFolder(FileBucket.cache) | ||
| ); | ||
|
|
||
| let totalSize = files.reduce( | ||
| (sum, file) => sum + parseInt(file.size as unknown as string, 10), | ||
| 0 | ||
| ); | ||
|
|
||
| // console.log('::::::::::::::::: REFERENCE_BIT', this.referenceBit); | ||
| // console.log('::::::::::::::::: CACHE', Object.fromEntries(cache)); | ||
|
|
||
| // Keep evicting least frequently used items until we're under capacity | ||
| let count = 0; | ||
| while (totalSize > this.capacityBytes) { | ||
| count++; | ||
|
|
||
| // Don't evict if it's among last files, could be single giant file | ||
| // Don't try more than 10 files at a time per eviction check. | ||
| if (files.length <= 2 || count > 10) { | ||
| break; | ||
| } | ||
|
|
||
| const evictedKey = this.findLFUKey(files, cache, triggerKey); | ||
| // console.log('::::::::::::: COUNT', count, ':::'); | ||
| // console.log('::::::::::::: EVICTKEY', count, evictedKey, ':::'); | ||
| // console.log('::::::::::::: FILES', count, files.length, ':::'); | ||
|
|
||
| if (!evictedKey) { | ||
| // Nothing left to evict or only the trigger file remains | ||
| break; | ||
| } | ||
|
|
||
| const cachedPath = cache.get(evictedKey); | ||
| // console.log('::::::::::::: CACHEPATH', count, cachedPath, ':::'); | ||
| if (!cachedPath) { | ||
| delete this.referenceBit[evictedKey]; // Clean up stale reference | ||
| continue; | ||
| } | ||
|
|
||
| // Find the file size we're about to evict | ||
| const fileToEvict = files.find((f) => cachedPath.includes(f.filename)); | ||
| if (!fileToEvict) { | ||
| // File doesn't exist on disk, clean up stale reference | ||
| cache.delete(evictedKey); | ||
| delete this.referenceBit[evictedKey]; | ||
| continue; | ||
| } | ||
|
|
||
| // Evict the file | ||
| cache.delete(evictedKey); | ||
| delete this.referenceBit[evictedKey]; | ||
| await delegate?.didEvictHandler(evictedKey, cachedPath); | ||
|
|
||
| // Update our running total | ||
| totalSize -= fileToEvict.size; | ||
| // file must exist or -1 will remove last item | ||
| files.splice(files.indexOf(fileToEvict), 1); | ||
|
|
||
| // console.log('::::::::::::: NewSize:', count, '||', totalSize, ':::'); | ||
| } | ||
| } finally { | ||
| this.isEvicting = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential array manipulation issue and remove debug logs
The eviction logic is well-structured, but there's a potential issue with array manipulation and debug code should be removed.
// Update our running total
totalSize -= fileToEvict.size;
- // file must exist or -1 will remove last item
- files.splice(files.indexOf(fileToEvict), 1);
+ const fileIndex = files.indexOf(fileToEvict);
+ if (fileIndex !== -1) {
+ files.splice(fileIndex, 1);
+ }Also remove the commented console.log statements on lines 78-79, 93-95, and 103, 128 for cleaner production code.
📝 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.
| async onEvict( | |
| cache: Map<string, any>, | |
| delegate?: MemoryCacheDelegate<any>, | |
| triggerKey?: string | |
| ) { | |
| if (this.isEvicting) { | |
| return; // Another eviction is in progress | |
| } | |
| try { | |
| this.isEvicting = true; | |
| // Get current directory size | |
| const files = await this.storage.getStatisticList( | |
| this.storage.getBucketFolder(FileBucket.cache) | |
| ); | |
| let totalSize = files.reduce( | |
| (sum, file) => sum + parseInt(file.size as unknown as string, 10), | |
| 0 | |
| ); | |
| // console.log('::::::::::::::::: REFERENCE_BIT', this.referenceBit); | |
| // console.log('::::::::::::::::: CACHE', Object.fromEntries(cache)); | |
| // Keep evicting least frequently used items until we're under capacity | |
| let count = 0; | |
| while (totalSize > this.capacityBytes) { | |
| count++; | |
| // Don't evict if it's among last files, could be single giant file | |
| // Don't try more than 10 files at a time per eviction check. | |
| if (files.length <= 2 || count > 10) { | |
| break; | |
| } | |
| const evictedKey = this.findLFUKey(files, cache, triggerKey); | |
| // console.log('::::::::::::: COUNT', count, ':::'); | |
| // console.log('::::::::::::: EVICTKEY', count, evictedKey, ':::'); | |
| // console.log('::::::::::::: FILES', count, files.length, ':::'); | |
| if (!evictedKey) { | |
| // Nothing left to evict or only the trigger file remains | |
| break; | |
| } | |
| const cachedPath = cache.get(evictedKey); | |
| // console.log('::::::::::::: CACHEPATH', count, cachedPath, ':::'); | |
| if (!cachedPath) { | |
| delete this.referenceBit[evictedKey]; // Clean up stale reference | |
| continue; | |
| } | |
| // Find the file size we're about to evict | |
| const fileToEvict = files.find((f) => cachedPath.includes(f.filename)); | |
| if (!fileToEvict) { | |
| // File doesn't exist on disk, clean up stale reference | |
| cache.delete(evictedKey); | |
| delete this.referenceBit[evictedKey]; | |
| continue; | |
| } | |
| // Evict the file | |
| cache.delete(evictedKey); | |
| delete this.referenceBit[evictedKey]; | |
| await delegate?.didEvictHandler(evictedKey, cachedPath); | |
| // Update our running total | |
| totalSize -= fileToEvict.size; | |
| // file must exist or -1 will remove last item | |
| files.splice(files.indexOf(fileToEvict), 1); | |
| // console.log('::::::::::::: NewSize:', count, '||', totalSize, ':::'); | |
| } | |
| } finally { | |
| this.isEvicting = false; | |
| } | |
| } | |
| // Update our running total | |
| totalSize -= fileToEvict.size; | |
| const fileIndex = files.indexOf(fileToEvict); | |
| if (fileIndex !== -1) { | |
| files.splice(fileIndex, 1); | |
| } |
🤖 Prompt for AI Agents
In src/Provider/MemoryCacheLFUSizePolicy.ts between lines 57 and 133, remove all
commented out console.log debug statements on lines 78-79, 93-95, 103, and 128
to clean up the production code. Additionally, ensure that when removing items
from the files array during eviction, the splice operation correctly targets the
intended index to avoid potential array manipulation issues.
| private findLFUKey( | ||
| files: Array<any>, | ||
| cache: Map<string, any>, | ||
| excludeKey?: string | ||
| ): string | null { | ||
| let minFreq = Number.MAX_VALUE; | ||
| let lfuKey: string | null = null; | ||
|
|
||
| for (const key in this.referenceBit) { | ||
| // Skip the file that triggered eviction | ||
| if (key === excludeKey) continue; | ||
|
|
||
| const freq = this.referenceBit[key]; | ||
| if (freq && freq < minFreq) { | ||
| if (freq !== SECOND_CHANCE_TO_COUNT || lfuKey === null) { | ||
| minFreq = freq; | ||
| lfuKey = key; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If all items have equal frequency, use the oldest file | ||
| if (!lfuKey && Object.keys(this.referenceBit).length > 0) { | ||
| const eligibleFiles = files.filter((file) => { | ||
| if (excludeKey) { | ||
| const excludePath = cache.get(excludeKey); | ||
| return !excludePath?.includes(file.filename); | ||
| } | ||
| return true; | ||
| }); | ||
|
|
||
| // Find the oldest file | ||
| const oldestFile = eligibleFiles.reduce((oldest, current) => { | ||
| return oldest.lastModified < current.lastModified ? oldest : current; | ||
| }); | ||
|
|
||
| // Find the referenceBit key that corresponds to this file | ||
| // Find which cache entry has this filename | ||
| lfuKey = | ||
| Array.from(cache.entries()).find(([_, path]) => | ||
| path.includes(oldestFile.filename) | ||
| )?.[0] || | ||
| cache.keys().next().value || // fallback to first (oldest) key | ||
| null; | ||
| } | ||
|
|
||
| return lfuKey; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks and simplify complex fallback logic
The method needs safety checks for edge cases and the fallback logic is overly complex.
});
+ if (eligibleFiles.length === 0) {
+ return null;
+ }
+
// Find the oldest file
const oldestFile = eligibleFiles.reduce((oldest, current) => {
return oldest.lastModified < current.lastModified ? oldest : current;
});Consider simplifying the complex fallback chain on lines 173-178 by extracting it to a separate method for better readability and maintainability.
🤖 Prompt for AI Agents
In src/Provider/MemoryCacheLFUSizePolicy.ts between lines 135 and 182, add
safety checks to handle cases where eligibleFiles might be empty before calling
reduce to avoid runtime errors. Simplify the fallback logic on lines 173-178 by
extracting the chain that finds lfuKey from cache entries or keys into a
separate helper method to improve readability and maintainability.
| set dataSource(data: { [key in string]: number }) { | ||
| const newDataSource = mergeWithCustomCondition( | ||
| this.referenceBit, | ||
| data, | ||
| mergeLargerNumber | ||
| ); | ||
| this.referenceBit = newDataSource; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for dataSource setter
The setter should validate the input data structure to prevent corruption of the internal state.
set dataSource(data: { [key in string]: number }) {
+ // Validate that all values are non-negative numbers
+ for (const key in data) {
+ if (typeof data[key] !== 'number' || data[key] < 0) {
+ throw new Error(`Invalid frequency value for key ${key}: ${data[key]}`);
+ }
+ }
const newDataSource = mergeWithCustomCondition(
this.referenceBit,
data,
mergeLargerNumber
);
this.referenceBit = newDataSource;
}📝 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.
| set dataSource(data: { [key in string]: number }) { | |
| const newDataSource = mergeWithCustomCondition( | |
| this.referenceBit, | |
| data, | |
| mergeLargerNumber | |
| ); | |
| this.referenceBit = newDataSource; | |
| } | |
| set dataSource(data: { [key in string]: number }) { | |
| // Validate that all values are non-negative numbers | |
| for (const key in data) { | |
| if (typeof data[key] !== 'number' || data[key] < 0) { | |
| throw new Error(`Invalid frequency value for key ${key}: ${data[key]}`); | |
| } | |
| } | |
| const newDataSource = mergeWithCustomCondition( | |
| this.referenceBit, | |
| data, | |
| mergeLargerNumber | |
| ); | |
| this.referenceBit = newDataSource; | |
| } |
🤖 Prompt for AI Agents
In src/Provider/MemoryCacheLFUSizePolicy.ts around lines 189 to 196, the
dataSource setter lacks validation for the input data structure, risking
internal state corruption. Add validation logic to check that the input is an
object with string keys and number values before merging. If the input is
invalid, throw an error or handle it gracefully to prevent corrupting
this.referenceBit.
| get memoryCache() { | ||
| return this._memoryCache; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider exposing the internal memory cache directly
Exposing the private _memoryCache through a public getter breaks encapsulation and could allow external code to bypass cache management logic. Consider providing specific methods for cache operations instead of exposing the entire cache object.
🤖 Prompt for AI Agents
In src/ProxyCacheManager.ts around lines 60 to 62, the public getter exposes the
private _memoryCache directly, breaking encapsulation. Remove the public getter
and instead implement specific methods that perform controlled cache operations
(e.g., get, set, delete) to interact with the cache while preserving
encapsulation and internal logic.
Pull Request
Description
Fixed some misc build issues, some still remain.
Added fixes for cases where there were stale cache keys. If something didn't exist in the filesystem and the key existed, it was just broken. Now it checks to make sure the file exists, if not it clears the key from memoryCache and that also clears the key from cachePolicy.
Added new ProxyCacheManager methods:
get memoryCache()- returns memoryCache for user useclearMemoryCache()- only clears memory cacheclearCache()- clears memory cache and deletes disk filesremoveCachedVideo(url)- removes a single url from memory and diskAdded to useAsyncCache:
removeVideoFromCache(url)to access from a component where the url may have been set. If anything happens to get corrupted this makes it possible to delete the cache and start over, previously it was just permanently broken.Fixed some cases in LFUPolicy where onEvict could fail if they all had equal frequency, now it falls back to first cache entry entered.
Added a new CachePolicy:
LFUSizePolicyThis works similarly to LFUPolicy, but when initializing it, the number is the size in MB for the cache. Now you can control how big the cache will get. It will grab a file list and delete the least frequented ones, falling back to the file with the oldest timestamp if that doesn't work. Additionally had to add more calls to onEvict since it wasn't ever called after first adding the files when they were small since they hadn't streamed much yet. Now it's also called on memoryCache get in addition to the put, extra calls shouldn't matter for other policies. Additionally, added an
isEvictingcheck toonEvictso if multiple video streams were running at once, it didn't attempt to do evictions at the same time.Type of Change
Checklist
The example project doesn't build, it's too old and hasn't been updated in too long. eslint and other checks and ts builds just fine. I added as many tests as already existed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes