fix: remove team members from filter segments when removed from team#24168
fix: remove team members from filter segments when removed from team#24168naaa760 wants to merge 3 commits intocalcom:mainfrom
Conversation
|
@naaa760 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a private static method Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
dosubot
bot
added
bookings
![coderabbitai[bot]](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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/lib/server/service/teamService.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/teamService.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/server/service/teamService.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/server/service/teamService.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/server/service/teamService.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
🧬 Code graph analysis (1)
packages/lib/server/service/teamService.ts (1)
packages/platform/libraries/index.ts (1)
TeamService(139-139)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
packages/lib/server/service/teamService.ts (3)
13-13: LGTM!The type import is properly structured and necessary for the new cleanup method.
244-244: LGTM!The cleanup is correctly positioned after workflow reminders cleanup and follows the established pattern of post-transaction cleanup operations.
438-497: Verify empty-array pruning and post-transaction cleanup
- The cleanup logic leaves an empty
msfilter when removing the lastuserId; there’s no backend pruning, only UI silencing. Confirm if empty arrays are acceptable or should be dropped likessfilters.- Cleanup runs after the removal transaction (e.g., lines 244, 435). If it fails, filters stay stale. Verify this post-transaction design aligns with requirements.
|
|
||
| await TeamService.cleanupFilterSegmentsWithRemovedUser(membership.userId, teamId); | ||
| } |
There was a problem hiding this comment.
Remove duplicate cleanup call.
The cleanupFilterSegmentsWithRemovedUser method is called twice for regular team member removals:
- Line 435: Inside
removeFromTeam - Line 244: In
removeMemberafter callingremoveFromTeam(line 239)
This results in redundant database queries and processing. Since removeMember already calls the cleanup method (line 244) after both removeFromOrganization and removeFromTeam, the call at line 435 should be removed.
Apply this diff to remove the duplicate:
- ]);
-
- await TeamService.cleanupFilterSegmentsWithRemovedUser(membership.userId, teamId);
+ ]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await TeamService.cleanupFilterSegmentsWithRemovedUser(membership.userId, teamId); | |
| } | |
| await prisma.teamMembership.deleteMany({ | |
| where: { userId: membership.userId, teamId }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/lib/server/service/teamService.ts around lines 434 to 436, the call
to TeamService.cleanupFilterSegmentsWithRemovedUser(membership.userId, teamId)
is duplicated (also invoked by removeMember at line 244 after removeFromTeam);
remove the call at line 435 so cleanup is only performed once by removeMember to
avoid redundant work.
|
This PR is being marked as stale due to inactivity. |
|
@kart1ka |
|
@cubic-dev-ai leave a review here. |
@pallava-joshi I've started the AI code review. It'll take a few minutes to complete. |
pallava-joshi
left a comment
pallava-joshi
left a comment
There was a problem hiding this comment.
can you please resolve the merge conflicts and implement the suggestions.
|
marking this draft until then. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/lib/server/service/teamService.ts">
<violation number="1" location="packages/lib/server/service/teamService.ts:381">
Cleanup only runs for the organization teamId, so child-team filter segments still reference the removed user even though their memberships were deleted. Iterate each child team (or extend cleanup to handle multiple ids) so sub-team segments are cleaned as well.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| await deleteWorkfowRemindersOfRemovedMember(team, userId, isOrg); | ||
|
|
||
| await TeamService.cleanupFilterSegmentsWithRemovedUser(userId, team.id); |
There was a problem hiding this comment.
Cleanup only runs for the organization teamId, so child-team filter segments still reference the removed user even though their memberships were deleted. Iterate each child team (or extend cleanup to handle multiple ids) so sub-team segments are cleaned as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/lib/server/service/teamService.ts, line 381:
<comment>Cleanup only runs for the organization teamId, so child-team filter segments still reference the removed user even though their memberships were deleted. Iterate each child team (or extend cleanup to handle multiple ids) so sub-team segments are cleaned as well.</comment>
<file context>
@@ -377,6 +378,8 @@ export class TeamService {
await deleteWorkfowRemindersOfRemovedMember(team, userId, isOrg);
+ await TeamService.cleanupFilterSegmentsWithRemovedUser(userId, team.id);
+
return { membership };
</file context>
naaa760
force-pushed
the
fix/team-filter-removal
branch
from
13cb5c2 to
2eaddaa
Compare
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/lib/server/service/teamService.ts">
<violation number="1" location="packages/lib/server/service/teamService.ts:441">
Removing a regular team member now triggers `cleanupFilterSegmentsWithRemovedUser` twice (once in `removeFromTeam`, once in `removeMember`), doubling the database work for each removal with no functional gain.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| }), | ||
| ]); | ||
|
|
||
| await TeamService.cleanupFilterSegmentsWithRemovedUser(membership.userId, teamId); |
There was a problem hiding this comment.
Removing a regular team member now triggers cleanupFilterSegmentsWithRemovedUser twice (once in removeFromTeam, once in removeMember), doubling the database work for each removal with no functional gain.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/lib/server/service/teamService.ts, line 441:
<comment>Removing a regular team member now triggers `cleanupFilterSegmentsWithRemovedUser` twice (once in `removeFromTeam`, once in `removeMember`), doubling the database work for each removal with no functional gain.</comment>
<file context>
@@ -428,5 +437,103 @@ export class TeamService {
}),
]);
+
+ await TeamService.cleanupFilterSegmentsWithRemovedUser(membership.userId, teamId);
+ }
+
</file context>
✅ Addressed in b5a83d3
pallava-joshi
left a comment
pallava-joshi
left a comment
There was a problem hiding this comment.
still got merge conflicts, cubic suggestions and failing type checks. lmk if you need any help with anything.
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/lib/server/service/teamService.ts">
<violation number="1" location="packages/lib/server/service/teamService.ts:534">
The catch block references an undefined `teamId`, so any exception in filter cleanup throws again during logging and prevents member removal from completing.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| } | ||
| } | ||
| } catch (error) { | ||
| log.error(`Failed to cleanup filter segments for removed user ${userId} from team ${teamId}`, error); |
There was a problem hiding this comment.
The catch block references an undefined teamId, so any exception in filter cleanup throws again during logging and prevents member removal from completing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/lib/server/service/teamService.ts, line 534:
<comment>The catch block references an undefined `teamId`, so any exception in filter cleanup throws again during logging and prevents member removal from completing.</comment>
<file context>
@@ -429,4 +438,100 @@ export class TeamService {
+ }
+ }
+ } catch (error) {
+ log.error(`Failed to cleanup filter segments for removed user ${userId} from team ${teamId}`, error);
+ }
+ }
</file context>
| log.error(`Failed to cleanup filter segments for removed user ${userId} from team ${teamId}`, error); | |
| log.error(`Failed to cleanup filter segments for removed user ${userId} from teams ${uniqueTeamIds.join(",")}`, error); |
|
closing this PR due to staleness, feel free to reopen if you wish to work on this further. thanks a ton for your contribution :) |
2 participants
closes: #24165
What does this PR do?