Optimize calendar cache performance and maintenance#10
Optimize calendar cache performance and maintenance#10ShashankFC wants to merge 1 commit intomainfrom
Conversation
Entelligence AI Vulnerability ScannerStatus: No security vulnerabilities found Your code passed our comprehensive security analysis. Analyzed 3 files in total |
Review Summary🏷️ Draft Comments (3)
|
🔬 Multi-Approach Review SummaryThis PR was reviewed by 2 different approaches for comparison:
Total: 5 review comments Each comment is labeled with its source approach. This allows you to compare different AI review strategies. 🔒 Security Scan: Run once and shared across all approaches for efficiency. WalkthroughThis PR implements comprehensive calendar cache management improvements across three key areas. First, it fixes a critical bug in cache expiration logic by changing comparison operators from Changes
Sequence DiagramThis diagram shows the interactions between components: sequenceDiagram
participant Client
participant CacheService
participant Database
Note over CacheService: New constant added:<br/>CACHE_CLEANUP_THRESHOLD = 7 days
Client->>CacheService: Request cached data (userId, key)
activate CacheService
CacheService->>Database: Query cache by userId and key
activate Database
Note over Database: Modified filter:<br/>expiresAt > now<br/>(was: expiresAt >= now)
alt Cache entry exists and not expired
Database-->>CacheService: Return cached data (ordered by expiresAt DESC)
CacheService-->>Client: Return cached result
else No valid cache entry
Database-->>CacheService: Return null
CacheService-->>Client: Cache miss
end
deactivate Database
deactivate CacheService
Client->>CacheService: Request cached data (credentialId, key)
activate CacheService
CacheService->>Database: Query cache by credentialId and key
activate Database
Note over Database: Modified filter:<br/>expiresAt > now<br/>(was: expiresAt >= now)
alt Cache entry exists and not expired
Database-->>CacheService: Return cached data
CacheService-->>Client: Return cached result
else No valid cache entry
Database-->>CacheService: Return null
CacheService-->>Client: Cache miss
end
deactivate Database
deactivate CacheService
Note over CacheService,Database: Expiry boundary fix:<br/>Entries expiring exactly now<br/>are now considered expired
🔒 Security Analysis
Note for WindsurfPlease change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
Also you can trigger various commands with the bot by doing The current supported commands are
More commands to be added soon. |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| async getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) { | ||
| if (typeof shouldServeCache === "boolean") return shouldServeCache; | ||
| if (!teamId) return false; | ||
| if (!teamId) return true; |
There was a problem hiding this comment.
correctness: 🟢 [Standard Reviewer] if (!teamId) return true; causes cache to be served even when teamId is missing, which may expose incorrect or unauthorized cache data.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In packages/features/calendar-cache/lib/getShouldServeCache.ts, on line 13, the code currently reads `if (!teamId) return true;`. This causes the cache to be served even when `teamId` is missing, which can result in serving incorrect or unauthorized cache data. Change this line to `if (!teamId) return false;` to ensure cache is only served when a valid teamId is present.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (!teamId) return true; | |
| if (!teamId) return false; |
| const MS_PER_DAY = 24 * 60 * 60 * 1000; | ||
| const ONE_MONTH_IN_MS = 30 * MS_PER_DAY; | ||
| const CACHING_TIME = ONE_MONTH_IN_MS; | ||
| const CACHE_CLEANUP_THRESHOLD = 7 * MS_PER_DAY; | ||
|
|
||
| function parseKeyForCache(args: FreeBusyArgs): string { | ||
| // Ensure that calendarIds are unique |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The constant CACHE_CLEANUP_THRESHOLD is defined but never used in the code. Remove it to clean up the codebase and avoid unnecessary clutter.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const MS_PER_DAY = 24 * 60 * 60 * 1000; | |
| const ONE_MONTH_IN_MS = 30 * MS_PER_DAY; | |
| const CACHING_TIME = ONE_MONTH_IN_MS; | |
| const CACHE_CLEANUP_THRESHOLD = 7 * MS_PER_DAY; | |
| function parseKeyForCache(args: FreeBusyArgs): string { | |
| // Ensure that calendarIds are unique | |
| const MS_PER_DAY = 24 * 60 * 60 * 1000; | |
| const ONE_MONTH_IN_MS = 30 * MS_PER_DAY; | |
| const CACHING_TIME = ONE_MONTH_IN_MS; | |
| function parseKeyForCache(args: FreeBusyArgs): string { | |
| // Ensure that calendarIds are unique |
| where: { | ||
| userId, | ||
| key, | ||
| expiresAt: { gte: new Date(Date.now()) }, | ||
| expiresAt: { gt: new Date(Date.now()) }, | ||
| }, | ||
| orderBy: { | ||
| // In case of multiple entries for same key and userId, we prefer the one with highest expiry, which will be the most updated one |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The change from gte to gt for expiresAt may exclude entries that expire exactly at the current time, potentially causing valid entries to be missed. Consider reverting to gte if entries expiring at the current time should be included.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| where: { | |
| userId, | |
| key, | |
| expiresAt: { gte: new Date(Date.now()) }, | |
| expiresAt: { gt: new Date(Date.now()) }, | |
| }, | |
| orderBy: { | |
| // In case of multiple entries for same key and userId, we prefer the one with highest expiry, which will be the most updated one | |
| where: { | |
| userId, | |
| key, | |
| expiresAt: { gte: new Date(Date.now()) }, | |
| }, | |
| orderBy: { | |
| // In case of multiple entries for same key and userId, we prefer the one with highest expiry, which will be the most updated one |
| credentialId, | ||
| key, | ||
| }, | ||
| expiresAt: { gte: new Date(Date.now()) }, | ||
| expiresAt: { gt: new Date(Date.now()) }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The change from gte to gt in the expiresAt condition ensures that only entries with a future expiration date are retrieved, which is likely the intended behavior. Verify that this change aligns with the business logic, as it excludes entries expiring at the current time.
|
|
||
| async getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) { | ||
| if (typeof shouldServeCache === "boolean") return shouldServeCache; | ||
| if (!teamId) return false; | ||
| if (!teamId) return true; | ||
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The async function getShouldServeCache lacks error handling for the awaited call to checkIfTeamHasFeature. This can lead to unhandled promise rejections if the call fails. Wrap the call in a try/catch block to handle potential errors.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| async getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) { | |
| if (typeof shouldServeCache === "boolean") return shouldServeCache; | |
| if (!teamId) return false; | |
| if (!teamId) return true; | |
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | |
| } | |
| } | |
| constructor(private readonly dependencies: ICacheService) {} | |
| async getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) { | |
| if (typeof shouldServeCache === "boolean") return shouldServeCache; | |
| if (!teamId) return true; | |
| try { | |
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | |
| } catch (error) { | |
| console.error('Error checking team feature:', error); | |
| return false; | |
| } | |
| } |
1 participant
This PR optimizes the calendar cache layer with improved query performance, better expiry handling, and automated cleanup utilities for maintaining cache health.
EntelligenceAI PR Summary
This PR enhances calendar cache management with bug fixes, cleanup utilities, and modified default caching behavior.
gtetogtin repository queriesCACHE_CLEANUP_THRESHOLDconstant set to 7 days