Conversation
- Update cache expiry time calculation for better coverage - Refine cache validation logic for team features - Optimize calendar sync filtering - Enhance organization admin check with detailed membership info
Entelligence AI Vulnerability ScannerStatus: No security vulnerabilities found Your code passed our comprehensive security analysis. Analyzed 4 files in total |
Review Summary❌ Rejected Comments (1)
🏷️ Draft Comments (3)
|
🔬 Multi-Approach Review SummaryThis PR was reviewed by 2 different approaches for comparison:
Total: 8 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 addresses calendar cache management and team feature validation logic. The primary changes include extending the cache duration from 30 to 31 days for accurate monthly representation, and refining cache expiry queries to use strict comparison operators to prevent edge cases with expired entries. A critical bug fix inverts the team ID validation logic in the cache serving method, ensuring feature checks occur when a team ID is present rather than absent. Additionally, the calendar synchronization filtering is simplified by removing the subscription timestamp requirement, relying solely on sync tokens. A minor refactoring improves code readability in the organization admin query function. Changes
Sequence DiagramThis diagram shows the interactions between components: sequenceDiagram
participant Client
participant CalendarCacheRepository
participant Database
Note over CalendarCacheRepository: Configuration<br/>CACHING_TIME = 31 days (updated from 30)
Client->>CalendarCacheRepository: getCachedData(userId, key)
activate CalendarCacheRepository
CalendarCacheRepository->>CalendarCacheRepository: parseKeyForCache(args)
CalendarCacheRepository->>Database: findFirst({<br/>where: userId, key,<br/>expiresAt > now })
Note over Database: Changed from gte to gt<br/>Entries expiring exactly now<br/>are now considered expired
alt Cache entry found and valid
Database-->>CalendarCacheRepository: Return cached entry
CalendarCacheRepository-->>Client: Return cached data
else No valid cache entry
Database-->>CalendarCacheRepository: Return null
CalendarCacheRepository-->>Client: Return null (cache miss)
end
deactivate CalendarCacheRepository
Note over Client,Database: Similar flow for credentialId-based lookup
Client->>CalendarCacheRepository: getCachedData(credentialId, key)
activate CalendarCacheRepository
CalendarCacheRepository->>Database: findUnique({<br/>where: credentialId, key,<br/>expiresAt > now })
Note over Database: Also changed from gte to gt
alt Cache entry found and valid
Database-->>CalendarCacheRepository: Return cached entry
CalendarCacheRepository-->>Client: Return cached data
else No valid cache entry
Database-->>CalendarCacheRepository: Return null
CalendarCacheRepository-->>Client: Return null (cache miss)
end
deactivate CalendarCacheRepository
🔒 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}
| if (teamId) return false; | ||
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId!, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); |
There was a problem hiding this comment.
correctness: 🟢 [Standard Reviewer] if (teamId) return false; inverts the intended logic: it returns false when teamId is present, preventing feature checks for valid teams and always returning false for valid input.
🤖 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, lines 13-14, the logic is inverted: `if (teamId) return false;` causes the function to return false when a valid teamId is provided, which is incorrect. Change it to `if (!teamId) return false;` and remove the non-null assertion on teamId in the next line. The corrected code should check for missing teamId and only then return false.
📝 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 false; | |
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId!, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | |
| if (!teamId) return false; | |
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); |
| const membership = await prisma.membership.findFirst({ | ||
| where: { | ||
| userId, | ||
| teamId: orgId, | ||
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | ||
| }, | ||
| }); | ||
| return membership || false; |
There was a problem hiding this comment.
correctness: 🟢 [Standard Reviewer] isOrganisationAdmin now returns the membership object or false, which may break callers expecting a boolean result and cause logic errors in permission checks.
🤖 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/lib/server/queries/organisations/index.ts, lines 8-15, the function `isOrganisationAdmin` now returns the membership object or `false`, which can break callers expecting a boolean and cause permission logic errors. Change the return statement to `return !!membership;` so the function always returns a boolean.
📝 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 membership = await prisma.membership.findFirst({ | |
| where: { | |
| userId, | |
| teamId: orgId, | |
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | |
| }, | |
| }); | |
| return membership || false; | |
| const membership = await prisma.membership.findFirst({ | |
| where: { | |
| userId, | |
| teamId: orgId, | |
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | |
| }, | |
| }); | |
| return !!membership; |
| const log = logger.getSubLogger({ prefix: ["CalendarCacheRepository"] }); | ||
|
|
||
| const MS_PER_DAY = 24 * 60 * 60 * 1000; | ||
| const ONE_MONTH_IN_MS = 30 * MS_PER_DAY; | ||
| const ONE_MONTH_IN_MS = 31 * MS_PER_DAY; | ||
| const CACHING_TIME = ONE_MONTH_IN_MS; | ||
|
|
||
| function parseKeyForCache(args: FreeBusyArgs): string { |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The constant ONE_MONTH_IN_MS is set to 31 days, which may not accurately represent a month. Consider using an average month duration or a more precise calculation based on the current month.
📝 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 log = logger.getSubLogger({ prefix: ["CalendarCacheRepository"] }); | |
| const MS_PER_DAY = 24 * 60 * 60 * 1000; | |
| const ONE_MONTH_IN_MS = 30 * MS_PER_DAY; | |
| const ONE_MONTH_IN_MS = 31 * MS_PER_DAY; | |
| const CACHING_TIME = ONE_MONTH_IN_MS; | |
| function parseKeyForCache(args: FreeBusyArgs): string { | |
| const log = logger.getSubLogger({ prefix: ["CalendarCacheRepository"] }); | |
| const MS_PER_DAY = 24 * 60 * 60 * 1000; | |
| const ONE_MONTH_IN_MS = 30.44 * MS_PER_DAY; // Average month duration | |
| const CACHING_TIME = ONE_MONTH_IN_MS; | |
| function parseKeyForCache(args: FreeBusyArgs): string { |
| 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 cache entries to be missed. Verify if this change is intentional.
📝 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] Change from gte to gt in expiresAt ensures only future entries are retrieved. Confirm this aligns with business logic.
|
|
||
| async getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) { | ||
| if (typeof shouldServeCache === "boolean") return shouldServeCache; | ||
| if (!teamId) return false; | ||
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | ||
| if (teamId) return false; | ||
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId!, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The use of teamId! is unsafe as it assumes teamId is always defined, which can lead to runtime errors if teamId is undefined. Consider adding a check or handling the case where teamId might be undefined.
📝 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; | |
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | |
| if (teamId) return false; | |
| 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 !== undefined) { | |
| return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, CalendarSubscriptionService.CALENDAR_SUBSCRIPTION_CACHE_FEATURE); | |
| } | |
| return false; | |
| } |
|
|
||
| if (!selectedCalendars?.length) return []; | ||
|
|
||
| const withSync = selectedCalendars.filter((c) => c.syncToken && c.syncSubscribedAt); | ||
| const withoutSync = selectedCalendars.filter((c) => !c.syncToken || !c.syncSubscribedAt); | ||
| const withSync = selectedCalendars.filter((c) => c.syncToken); | ||
| const withoutSync = selectedCalendars.filter((c) => !c.syncToken); | ||
|
|
||
| const results: EventBusyDate[] = []; | ||
|
|
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] Align the filtering logic in getAvailability with getAvailabilityWithTimeZones by checking both syncToken and syncSubscribedAt.
📝 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 (!selectedCalendars?.length) return []; | |
| const withSync = selectedCalendars.filter((c) => c.syncToken && c.syncSubscribedAt); | |
| const withoutSync = selectedCalendars.filter((c) => !c.syncToken || !c.syncSubscribedAt); | |
| const withSync = selectedCalendars.filter((c) => c.syncToken); | |
| const withoutSync = selectedCalendars.filter((c) => !c.syncToken); | |
| const results: EventBusyDate[] = []; | |
| }); | |
| if (!selectedCalendars?.length) return []; | |
| const withSync = selectedCalendars.filter((c) => c.syncToken && c.syncSubscribedAt); | |
| const withoutSync = selectedCalendars.filter((c) => !c.syncToken || !c.syncSubscribedAt); | |
| const results: EventBusyDate[] = []; | |
|
|
||
| // also returns team | ||
| export async function isOrganisationAdmin(userId: number, orgId: number) { | ||
| return ( | ||
| (await prisma.membership.findFirst({ | ||
| where: { | ||
| userId, | ||
| teamId: orgId, | ||
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | ||
| }, | ||
| })) || false | ||
| ); | ||
| const membership = await prisma.membership.findFirst({ | ||
| where: { | ||
| userId, | ||
| teamId: orgId, | ||
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | ||
| }, | ||
| }); | ||
| return membership || false; | ||
| } | ||
| export async function isOrganisationOwner(userId: number, orgId: number) { | ||
| return !!(await prisma.membership.findFirst({ |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The isOrganisationAdmin function lacks error handling for the async operation. Wrap the await call in a try/catch block to handle potential errors from the database query.
📝 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.
| // also returns team | |
| export async function isOrganisationAdmin(userId: number, orgId: number) { | |
| return ( | |
| (await prisma.membership.findFirst({ | |
| where: { | |
| userId, | |
| teamId: orgId, | |
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | |
| }, | |
| })) || false | |
| ); | |
| const membership = await prisma.membership.findFirst({ | |
| where: { | |
| userId, | |
| teamId: orgId, | |
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | |
| }, | |
| }); | |
| return membership || false; | |
| } | |
| export async function isOrganisationOwner(userId: number, orgId: number) { | |
| return !!(await prisma.membership.findFirst({ | |
| // export type OrganisationWithMembers = Awaited<ReturnType<typeof getOrganizationMembers>>; | |
| // also returns team | |
| export async function isOrganisationAdmin(userId: number, orgId: number) { | |
| try { | |
| const membership = await prisma.membership.findFirst({ | |
| where: { | |
| userId, | |
| teamId: orgId, | |
| OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }], | |
| }, | |
| }); | |
| return membership || false; | |
| } catch (error) { | |
| console.error('Error fetching membership:', error); | |
| return false; | |
| } | |
| } | |
| export async function isOrganisationOwner(userId: number, orgId: number) { | |
| return !!(await prisma.membership.findFirst({ | |
| where: { | |
| userId, | |
| teamId: orgId, | |
| role: MembershipRole.OWNER, | |
| }, | |
| })); | |
| } |
1 participant
This PR enhances the cache validation system with better expiry time calculations and improved team feature checks. It also optimizes calendar sync filtering for better performance.
EntelligenceAI PR Summary
This PR fixes calendar cache expiry handling and corrects a critical team feature validation bug, while simplifying sync logic and improving code readability.
gtetogtto properly handle exact timestamp matchesteamIdvalidation logic ingetShouldServeCachemethodsyncSubscribedAtrequirement from calendar sync filtering, relying only onsyncTokenisOrganisationAdminfor better code clarity