Conversation
|
Warning Rate limit exceeded@aaandreyev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a FrameCacheManager module with memory- and size-aware eviction, refactors use-frame-cache to delegate caching, adds tests for cache behavior, and updates CI to run Changes
Sequence Diagram(s)sequenceDiagram
participant Component as React Component
participant Hook as useFrameCache Hook
participant Manager as FrameCacheManager
participant Shared as Shared Resources
Component->>Hook: call useFrameCache(options)
Hook->>Hook: normalizeOptions(), create/get manager
Note over Hook,Manager: Cache Lookup
Component->>Hook: isFrameCached(time, sceneId)
Hook->>Manager: isFrameCached(frameKey, timelineHash)
Manager-->>Hook: hit / miss
alt Cache Hit
Component->>Hook: getCachedFrame(...)
Hook->>Manager: getCachedFrame(frameKey, timelineHash)
Manager->>Shared: read Map
Manager-->>Hook: CachedFrame (ImageData)
Hook-->>Component: ImageData
else Cache Miss
Component->>Hook: renderFrame()
Hook->>Hook: schedule cache via requestIdleCallback
Hook->>Manager: cacheFrame(frameKey, timelineHash, imageData)
Manager->>Manager: ensureCapacity()
alt Within limits
Manager->>Shared: store entry, update memoryUsage
else Overflow
Manager->>Manager: evict oldest entries
Manager->>Shared: remove entries, update memoryUsage
Manager->>Shared: store entry
end
Manager-->>Hook: done
end
Note over Hook,Manager: Invalidation
Component->>Hook: timeline/project state changed
Hook->>Manager: invalidateCache() / clear()
Manager->>Shared: clear Map, reset counters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/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: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/bun-ci.yml(1 hunks)apps/web/src/hooks/__tests__/frame-cache-manager.test.ts(1 hunks)apps/web/src/hooks/frame-cache-manager.ts(1 hunks)apps/web/src/hooks/use-frame-cache.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/src/hooks/__tests__/frame-cache-manager.test.ts (2)
apps/web/src/hooks/frame-cache-manager.ts (3)
DEFAULT_FRAME_CACHE_OPTIONS(27-31)FrameCacheSharedResources(22-25)FrameCacheManager(161-398)apps/web/src/types/timeline.ts (1)
TimelineTrack(83-90)
apps/web/src/hooks/frame-cache-manager.ts (3)
apps/web/src/types/timeline.ts (3)
TimelineTrack(83-90)MediaElement(18-22)TextElement(25-40)apps/web/src/types/media.ts (1)
MediaFile(4-17)apps/web/src/types/project.ts (1)
TProject(13-35)
apps/web/src/hooks/use-frame-cache.ts (4)
apps/web/src/hooks/frame-cache-manager.ts (7)
FrameCacheOptions(16-20)normalizeOptions(39-49)getSharedResources(51-69)FrameCacheManager(161-398)isFrameCached(306-329)getCachedFrame(331-357)cacheResolution(184-186)apps/web/src/types/timeline.ts (1)
TimelineTrack(83-90)apps/web/src/types/media.ts (1)
MediaFile(4-17)apps/web/src/types/project.ts (1)
TProject(13-35)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/hooks/frame-cache-manager.ts (2)
256-268: Consider optimizing oldest-key lookup for larger caches.The O(n) linear search is acceptable for the default maxCacheSize of 300, but could become a bottleneck with larger caches. If performance becomes an issue, consider maintaining a min-heap or using an ordered data structure.
363-396: Consider validating frame size before caching.The logic correctly replaces existing frames and enforces capacity limits. However, if
imageData.datais undefined or empty (line 379), a 0-byte cache entry would be stored. Consider skipping the cache operation ifnewFrameSize === 0to avoid storing invalid frames.const newFrameSize = imageData.data?.byteLength ?? 0; + if (newFrameSize === 0) { + return; + } + if (this.cache.has(frameKey)) { this.removeFrame(frameKey); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/__tests__/frame-cache-manager.test.ts(1 hunks)apps/web/src/hooks/frame-cache-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/hooks/tests/frame-cache-manager.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/frame-cache-manager.ts (4)
apps/web/src/hooks/use-frame-cache.ts (1)
FrameCacheOptions(14-14)apps/web/src/types/timeline.ts (3)
TimelineTrack(83-90)MediaElement(18-22)TextElement(25-40)apps/web/src/types/media.ts (1)
MediaFile(4-17)apps/web/src/types/project.ts (1)
TProject(13-35)
🔇 Additional comments (12)
apps/web/src/hooks/frame-cache-manager.ts (12)
5-25: LGTM! Well-structured type definitions.The interfaces are clean and comprehensive, providing strong typing for the cache implementation.
27-49: LGTM! Sensible defaults and robust option normalization.The default cache limits and option handling are well-implemented.
51-69: LGTM! Proper global shared state management.The lazy initialization of shared resources via globalThis enables cache sharing across manager instances while maintaining type safety.
71-159: LGTM! Comprehensive active element computation.The function correctly identifies active elements at a given time, properly handling muted tracks, hidden elements, and trim offsets. Only media and text element types are explicitly handled; other types are silently skipped.
161-202: LGTM! Clean constructor and accessor pattern.The dependency injection of the
now()function is excellent for testability, and the getters provide appropriate read-only access to cache state.
204-245: LGTM! Thorough timeline state hashing.The frame key quantization and comprehensive hash generation correctly capture all state that affects rendering. The clampedTime ensures hash stability within cache resolution boundaries.
247-254: LGTM! Safe frame removal with defensive memory accounting.The fallback chain for size and the Math.max guard prevent memory tracking from going negative even if cache state is inconsistent.
270-302: LGTM! Robust capacity management with proper fix applied.The past review concern has been addressed: oversized frames are now rejected without clearing the entire cache (lines 279-281). The eviction loop correctly enforces both size and memory limits.
The final capacity check (lines 294-299) serves as a safety net and should rarely trigger if eviction logic is working correctly.
304-308: LGTM! Simple and correct LRU timestamp update.
310-361: LGTM! Proper cache validation with automatic invalidation.Both methods correctly validate cached frames against current timeline state. Note that
isFrameCacheddoesn't update the LRU timestamp (line 330 removes stale entries but doesn't touch valid ones), whilegetCachedFramedoes (line 359). This is likely intentional—checking cache validity shouldn't affect eviction order.
398-401: LGTM! Clean cache reset.
404-404: LGTM! Proper type re-export for external consumers.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_6904c8bd5ad48323876d94ce55cc7519
Summary by CodeRabbit
New Features
Tests
Chores