fix: Issues observed when inviting members to organization either through teams migration during onboarding or explicit user invite#22901
Conversation
Instead of processing team migrations in parallel using Promise.all, process them sequentially to avoid race conditions when creating profiles and memberships for team members. This simplifies the implementation and avoids the need for complex retry logic or database-level concurrency controls.
…in it as the first item. This fixes the issue where these promises were unhandled
…nding even though theorganization membership gets created as accepted
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
WalkthroughThis change set modifies the logic for migrating teams to process them sequentially rather than in parallel, addressing unique constraint issues during membership and profile creation. It introduces a new integration test suite for the invite member handler, focusing on organizational membership and profile scenarios, and expands unit tests with additional fields and organizational state coverage. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All code changes are relevant to the objectives in the linked issue, focusing on membership/profile handling and organizational invite logic. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
keithwillcode
added
core
hariombalhara
changed the title
E2E results are ready! |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/07/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add platform team as reviewer" took an action on this PR • (08/07/25)1 reviewer was added to this PR based on Keith Williams's automation. |
dosubot
bot
added
platform
![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
🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts (1)
177-177: Use strict equality for consistencyFor consistency with the rest of the codebase and to avoid potential type coercion issues, use strict equality.
- return profile.organizationId != team.parentId; + return profile.organizationId !== team.parentId;packages/trpc/server/routers/viewer/organizations/createTeams.handler.ts (1)
98-109: Correct fix for race condition issues!Changing from parallel to sequential processing effectively prevents unique constraint violations when users belong to multiple teams being migrated. This is a necessary trade-off for data consistency.
Consider monitoring the performance impact of sequential processing for organizations with many teams. If this becomes a bottleneck, you could batch teams in smaller groups or implement a queue-based approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/trpc/server/routers/viewer/organizations/createTeams.handler.ts(1 hunks)packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.integration-test.ts(1 hunks)packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.test.ts(4 hunks)packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts(6 hunks)packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.test.tspackages/trpc/server/routers/viewer/organizations/createTeams.handler.tspackages/trpc/server/routers/viewer/teams/inviteMember/utils.tspackages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.integration-test.tspackages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.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/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.test.tspackages/trpc/server/routers/viewer/organizations/createTeams.handler.tspackages/trpc/server/routers/viewer/teams/inviteMember/utils.tspackages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.integration-test.tspackages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.test.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Applied to files:
packages/trpc/server/routers/viewer/organizations/createTeams.handler.tspackages/trpc/server/routers/viewer/teams/inviteMember/utils.tspackages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.integration-test.ts
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
🪛 GitHub Actions: PR Update
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.integration-test.ts
[error] 119-119: TypeScript error TS2322: Type 'string | undefined' is not assignable to type 'string'.
🔇 Additional comments (8)
packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts (3)
163-178: Good refactoring for improved readability!The extraction of profile checking logic into
hasDifferentOrganizationProfilemakes the code more maintainable and easier to understand. The three distinct cases are now clearly documented.
848-862: Promise.all handles empty arrays correctlyPassing the mapped promise array directly to
Promise.all(instead of wrapping it in[...]) does not change behavior—when the array is empty,Promise.all([])resolves immediately with[]. This is consistent with howPromise.allis used elsewhere in the codebase, so no further changes are needed.
897-912: Confirm correct targeting of sub-team membershipsThe
updateManycall inpackages/trpc/server/routers/viewer/teams/inviteMember/utils.tsuses the filterteam: { parentId: organization.id }to accept only pending memberships for direct sub-teams when
shouldAutoAcceptis true. This aligns with existing patterns and ensures membership state consistency.• File: packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
• Lines: 900–907packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.test.ts (1)
1-5: Good practice to deprecate in favor of integration tests!The deprecation notice clearly directs developers to the integration test file, which provides better test coverage with less mocking.
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts (1)
471-489: Good test coverage for profile edge case!This test case covers an important scenario where a user has a profile with an organization but no team membership, ensuring they can still be invited.
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.integration-test.ts (3)
54-79: Well-structured test helper with proper cleanup!The
createTestUserhelper properly handles unique ID generation and pre-emptive cleanup to avoid conflicts. The error handling with empty catch blocks is appropriate for cleanup operations.
160-193: Excellent test cleanup strategy!The cleanup in
afterEachfollows the correct dependency order and uses transactions for atomicity. This ensures test isolation and prevents data leakage between tests.
358-436: Comprehensive test for the key fix!This test effectively validates the PR's main objective - ensuring that pending sub-team memberships are auto-accepted when a user with matching email domain is invited to the organization. Great coverage of the critical path.
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.integration-test.ts
Show resolved
Hide resolved
3 participants
What does this PR do?
Fixes following issues
Tests
Mandatory Tasks (DO NOT REMOVE)