-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: Remove filter cache #2769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: alpha
Are you sure you want to change the base?
fix: Remove filter cache #2769
Conversation
I will reformat the title to use the proper commit message syntax. |
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
Uffizzi Ephemeral Environment
|
WalkthroughThe update removes the in-memory caching mechanism for class preferences in the Changes
Assessment against linked issues
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/ClassPreferences.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
cache[appId] = cache[appId] || {}; | ||
cache[appId][className] = prefs; | ||
return prefs; | ||
return JSON.parse(entry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Removing cache simplifies the code but may slightly impact performance
The code now directly returns the parsed JSON from localStorage without using an in-memory cache. This simplification aligns with the PR objective to remove the filter cache, which makes the code more predictable and reduces memory usage.
While this change simplifies the code, it might slightly impact performance as each preference read now requires parsing JSON from localStorage. For most use cases, this performance difference would be negligible, but if preference fetching becomes a performance bottleneck in the future, consider implementing a more sophisticated caching strategy with proper invalidation.
New Pull Request Checklist
Issue Description
Closes: #2711
Approach
TODOs before merging
Summary by CodeRabbit