Feature/fusion cache#501
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds ZiggyCreatures.FusionCache integration and a WebApplicationBuilder extension to register it; migrates several cache usages from IMemoryCache to IFusionCache or alternate in-memory strategies; updates ProfileCache API (invalidate vs set) and related Razor call sites; adds NuGet package reference. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip We've launched Issue Planner and it is currently in beta. Please try it out and share your feedback on Discord! 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/web/Jordnaer/Features/Ad/AdProvider.cs`:
- Around line 32-56: The cached result is built using DateTime.UtcNow inside the
fusionCache factory (fusionCache.GetOrSetAsync) so time-windowed partners can be
stale for the full cache TTL; fix by either (A) making the cache value a
superset and applying the active-window filter at read time (i.e., after
partnerAds is returned, filter by DateTime.UtcNow vs
DisplayStartUtc/DisplayEndUtc) or (B) compute the nearest upcoming
DisplayStartUtc/DisplayEndUtc from the DB inside the factory and set the cache
expiration to that delta so the factory refreshes when visibility changes;
update the code paths around partnerAds and the factory passed to
fusionCache.GetOrSetAsync and ensure comparisons use DateTime.UtcNow and the
Partner.DisplayStartUtc/DisplayEndUtc properties.
In `@src/web/Jordnaer/Features/Chat/ChatMessageCache.cs`:
- Around line 25-40: The cache-hit path in ChatMessageCache is racy: multiple
concurrent callers can read the same cachedMessages.Count and both fetch and
AddRange identical new messages; fix by serializing per-chatId incremental loads
in the method that contains the shown block (the cache-hit path) — either add a
ConcurrentDictionary<chatId, SemaphoreSlim> or reuse the existing _inflightLoads
pattern to ensure only one fetch-and-merge runs per chatId at a time, await that
lock before calling chatService.GetChatMessagesAsync, then re-read the current
cached list and compute the real delta (e.g., snapshot currentCount and append
only messages with index >= currentCount) before calling _cache.AddOrUpdate or
replacing the entry to avoid duplicates.
In `@src/web/Jordnaer/Jordnaer.csproj`:
- Line 59: Update the ZiggyCreatures.FusionCache PackageReference from
Version="2.0.0" to Version="2.5.0" in the project file entry that contains
PackageReference Include="ZiggyCreatures.FusionCache"; after changing the
version, run dotnet restore (or your CI restore step) and execute tests/build to
verify compatibility and address any API changes introduced between FusionCache
2.0.0 and 2.5.0.
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/Chat/ChatMessageCache.cs (1)
56-69: Loading all messages (take: int.MaxValue) could be expensive for long-lived chats.Both the initial load and the incremental fetch use
int.MaxValueas thetakeparameter. For chats with large message histories, this will load the entire history into memory per circuit. Consider paginating or capping the initial load.src/web/Jordnaer/Features/Profile/ProfileCache.cs (1)
22-23:UserTagas a computed property recalculates on every access.This is fine since
currentUser.Idis stable within a circuit-scoped lifetime, but a minor consideration: ifRemoveByTagor tag assignment is called frequently, you're allocating a new string each time.Consider caching the tag string
private const string Tag = "profile"; -private string UserTag => $"{Tag}:{currentUser.Id}"; +private string? _userTag; +private string UserTag => _userTag ??= $"{Tag}:{currentUser.Id}";
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/web/Jordnaer/Features/Chat/ChatMessageCache.cs`:
- Around line 56-79: The method RefreshMessagesAsync (and similarly
LoadMessagesAsync) currently discards the Error<string> branch from
chatService.GetChatMessagesAsync, causing silent failures and stale/empty
returns; inject an ILogger<ChatMessageCache> into the ChatMessageCache class
and, when calling GetChatMessagesAsync inside RefreshMessagesAsync (and
LoadMessagesAsync), handle the error variant by logging the error with context
(userId, chatId) and either rethrowing or returning an appropriate failure
signal to the caller instead of swallowing it; update the Match(...) usages that
currently use "_" to capture the error and replace them with logic that logs via
the injected logger and then decides whether to propagate (throw) or surface an
error result to the caller.
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/Ad/AdProvider.cs (1)
50-72: Consider extracting the cache key to a constant for consistency with the Tag pattern.The code defines
Tag = "ads"as a constant (line 36) for the cache invalidation tag, but uses the inline string"PartnerAds"for the cache key (line 51). For consistency and to reduce the risk of accidental typos, extract the cache key to a named constant:Suggested change
+ private const string PartnerAdsCacheKey = "PartnerAds"; private const string Tag = "ads";Then update line 51:
- cachedPartners = await fusionCache.GetOrSetAsync<List<PartnerAdEntry>>( - "PartnerAds", + cachedPartners = await fusionCache.GetOrSetAsync<List<PartnerAdEntry>>( + PartnerAdsCacheKey,FusionCache defaults are properly configured during service registration (
WebApplicationBuilderExtensions.cs) with a 10-minute default duration, fail-safe enabled, and eager refresh at 90%. No explicit options are needed here.src/web/Jordnaer/Features/Chat/ChatMessageCache.cs (1)
81-94: Loading unbounded messages withint.MaxValuecould be a concern for large chats.Both
LoadMessagesAsync(line 86) andRefreshMessagesAsync(line 61) usetake: int.MaxValue, which fetches the entire chat history into memory. For a circuit-scoped cache this may be acceptable today, but worth keeping in mind if chat histories grow large.
No description provided.