fix: remove platform client ID from attendee list when adding team members as attendee in a team event #23866
Conversation
…mbers as attendee in a team event
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
keithwillcode
added
core
dosubot
bot
added
teams
| ...calEvent.team, | ||
| members: calEvent.team.members.map((member) => ({ | ||
| ...member, | ||
| email: member.email.replace(`+${clientId}`, ""), |
There was a problem hiding this comment.
remove the cliend it from the team members email
| @@ -42,7 +42,7 @@ export const useOAuthFlow = ({ accessToken, refreshUrl, clientId, onError, onSuc | |||
|
|
|||
| setIsRefreshing(false); | |||
| } | |||
| return Promise.reject(err.response); | |||
| return Promise.reject(err); | |||
There was a problem hiding this comment.
To fix the crash that occurs when changing the user from the dropdown in the example app,
Currently, we reject err.response, but on lines 62-63 of this file, we are attempting to access err.response.status, causing the crash
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/platform/atoms/hooks/useOAuthFlow.ts (1)
27-45: Fix: isRefreshing never reset on successful retry (refresh flow stays latched).On the success path you
returnbeforesetIsRefreshing(false), leaving the hook in a permanent refreshing state and blocking future refresh attempts.Apply this diff to ensure we always reset in a
finally, and to guard against undefined headers when retrying:- ? http.responseInterceptor.use(undefined, async (err: AxiosError) => { + ? http.responseInterceptor.use(undefined, async (err: AxiosError) => { const originalRequest = err.config as AxiosRequestConfig; if (refreshUrl && err.response?.status === 498 && !isRefreshing) { - setIsRefreshing(true); - const refreshedToken = await debouncedRefresh(refreshUrl); - if (refreshedToken) { - setClientAccessToken(refreshedToken); - onSuccess?.(); - return http.instance({ - ...originalRequest, - headers: { ...originalRequest.headers, Authorization: `Bearer ${refreshedToken}` }, - }); - } else { - onError?.("Invalid Refresh Token."); - } - - setIsRefreshing(false); + setIsRefreshing(true); + try { + const refreshedToken = await debouncedRefresh(refreshUrl); + if (refreshedToken) { + // Keep hook state and retry request with new token + setClientAccessToken(refreshedToken); + onSuccess?.(); + return http.instance({ + ...originalRequest, + headers: { ...(originalRequest.headers ?? {}), Authorization: `Bearer ${refreshedToken}` }, + }); + } else { + onError?.("Invalid Refresh Token."); + } + } finally { + setIsRefreshing(false); + } } return Promise.reject(err); })
🧹 Nitpick comments (2)
packages/lib/formatCalendarEvent.ts (2)
17-26: Sanitize only the local-part and handle multiple occurrences of +clientId.
string.replaceremoves only the first occurrence and may also touch the domain if it ever contained+clientId. Strip from the local-part before the@and remove all occurrences.Apply this diff within this block:
- const team = calEvent.team - ? { - ...calEvent.team, - members: calEvent.team.members.map((member) => ({ - ...member, - email: member.email.replace(`+${clientId}`, ""), - })), - } - : undefined; + const team = calEvent.team + ? { + ...calEvent.team, + members: calEvent.team.members.map((member) => ({ + ...member, + email: stripClientIdFromEmail(member.email, clientId), + })), + } + : undefined;And similarly above in this function for attendees and organizer:
- const attendees = calEvent.attendees.map((attendee) => ({ + const attendees = calEvent.attendees.map((attendee) => ({ ...attendee, - email: attendee.email.replace(`+${clientId}`, ""), + email: stripClientIdFromEmail(attendee.email, clientId), })); - const organizer = { + const organizer = { ...calEvent.organizer, - email: calEvent.organizer.email.replace(`+${clientId}`, ""), + email: stripClientIdFromEmail(calEvent.organizer.email, clientId), };Helper to add near the top of the file (outside selected range):
// Removes every "+<clientId>" token from the local-part only. const stripClientIdFromEmail = (email: string, clientId: string) => { const at = email.indexOf("@"); if (at === -1) return email.split(`+${clientId}`).join(""); const local = email.slice(0, at); const domain = email.slice(at + 1); const sanitizedLocal = local.split(`+${clientId}`).join(""); return `${sanitizedLocal}@${domain}`; };
32-34: Don’t assign team: undefined — preserve semantics of “no team” vs “team present”.Assigning an explicit
undefinedmay break “presence” checks. Only attachteamwhen it exists.- const [attendees, organizer, team] = formatClientIdFromEmails(clonedEvent, clonedEvent.platformClientId); - Object.assign(clonedEvent, { attendees, organizer, team }); + const [attendees, organizer, team] = formatClientIdFromEmails(clonedEvent, clonedEvent.platformClientId); + Object.assign(clonedEvent, team ? { attendees, organizer, team } : { attendees, organizer });
📜 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 (2)
packages/lib/formatCalendarEvent.ts(1 hunks)packages/platform/atoms/hooks/useOAuthFlow.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/platform/atoms/hooks/useOAuthFlow.tspackages/lib/formatCalendarEvent.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/platform/atoms/hooks/useOAuthFlow.tspackages/lib/formatCalendarEvent.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/platform/atoms/hooks/useOAuthFlow.tspackages/lib/formatCalendarEvent.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.192Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts:118-143
Timestamp: 2025-08-08T09:29:11.681Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (TypeScript), the AIPhoneServiceAgentListItem is required to include user.email in listAgents responses (per maintainer Udit-takkar). Future reviews should not flag this as unnecessary PII unless requirements change.
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/lib/formatCalendarEvent.ts
📚 Learning: 2025-08-27T16:39:38.192Z
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.192Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
Applied to files:
packages/lib/formatCalendarEvent.ts
🧬 Code graph analysis (1)
packages/lib/formatCalendarEvent.ts (1)
packages/types/Calendar.d.ts (1)
CalendarEvent(163-226)
🔇 Additional comments (2)
packages/platform/atoms/hooks/useOAuthFlow.ts (2)
45-46: Approve: rejecting AxiosError (err) is safe — downstream handlers expect AxiosError, not AxiosResponse.Search found catch handlers typed as (err: AxiosError) that check err.response?.status and no occurrences of direct error.status or catch parameters typed as AxiosResponse (see packages/platform/atoms/hooks/useOAuthClient.ts ~48–51 and useOAuthFlow.ts ~27–31, ~61–65).
27-45: Resolved — refreshTokens already updates the global Authorization header.
http.refreshTokens calls http.setAuthorizationHeader(res.accessToken) (packages/platform/atoms/lib/http.ts), so adding http.setAuthorizationHeader(refreshedToken) in the interceptor is redundant.
| const [attendees, organizer, team] = formatClientIdFromEmails(clonedEvent, clonedEvent.platformClientId); | ||
| Object.assign(clonedEvent, { attendees, organizer, team }); | ||
| } |
There was a problem hiding this comment.
Parity: also sanitize team on ExtendedCalendarEvent.
The hook now sanitizes team.members for CalendarEvent but not for ExtendedCalendarEvent, risking leaks of +clientId in flows using the extended shape (e.g., reminder schedulers).
Proposed change below (not in this hunk) for Lines 41-44:
- if (clonedEvent.platformClientId) {
- const [attendees, organizer] = formatClientIdFromEmails(clonedEvent, clonedEvent.platformClientId);
- Object.assign(clonedEvent, { attendees, organizer });
- }
+ if (clonedEvent.platformClientId) {
+ const [attendees, organizer, team] = formatClientIdFromEmails(clonedEvent, clonedEvent.platformClientId);
+ Object.assign(clonedEvent, team ? { attendees, organizer, team } : { attendees, organizer });
+ }📝 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.
| const [attendees, organizer, team] = formatClientIdFromEmails(clonedEvent, clonedEvent.platformClientId); | |
| Object.assign(clonedEvent, { attendees, organizer, team }); | |
| } | |
| if (clonedEvent.platformClientId) { | |
| const [attendees, organizer, team] = formatClientIdFromEmails(clonedEvent, clonedEvent.platformClientId); | |
| Object.assign(clonedEvent, team ? { attendees, organizer, team } : { attendees, organizer }); | |
| } |
🤖 Prompt for AI Agents
In packages/lib/formatCalendarEvent.ts around lines 32 and 41-44, the code
sanitizes team.members for CalendarEvent but not for ExtendedCalendarEvent;
update the ExtendedCalendarEvent handling to apply the same
formatClientIdFromEmails transformation so team.members are sanitized too.
Specifically, when constructing the extended shape, call
formatClientIdFromEmails with the cloned extended event and its platformClientId
(or reuse the same sanitization used for CalendarEvent) and assign the returned
attendees, organizer, and team onto the cloned extended event so team.members no
longer contain +clientId values.
E2E results are ready! |
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist