refactor: add logging in getCalEventResponses#23420
Conversation
WalkthroughThe change updates getCalEventResponses in packages/features/bookings/lib/getCalEventResponses.ts. It adds structured logging via a sub-logger prefixed "[getCalEventResponses]". The function’s input object now optionally includes responses (typed as z.infer) and seatsEnabled. Response selection prefers the provided responses, falling back to booking data. If email is absent and attendeePhoneNumber is not a string, it logs an error with context and throws a 400 HttpError. If attendeePhoneNumber is a string, email is constructed from it. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
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
Ryukemeister
changed the title
dosubot
bot
added
bookings
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/bookings/lib/getCalEventResponses.ts (1)
15-40: Clone inputresponsesto prevent unintended mutations
Wrap the resolvedbackwardCompatibleResponsesin a shallow copy before assigning to// before mutation const original = responses ?? getBookingWithResponses(booking!).responses; const backwardCompatibleResponses = { ...original }; // clone // then safely set placeholder email if (!backwardCompatibleResponses.email) { backwardCompatibleResponses.email = constructEmailFromPhoneNumber( backwardCompatibleResponses.attendeePhoneNumber ); }
🧹 Nitpick comments (1)
packages/features/bookings/lib/getCalEventResponses.ts (1)
61-63: Trim phone before deriving email (minor hardening).Apply:
- backwardCompatibleResponses["attendeePhoneNumber"] + backwardCompatibleResponses["attendeePhoneNumber"].trim()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/bookings/lib/getCalEventResponses.ts(2 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/features/bookings/lib/getCalEventResponses.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/features/bookings/lib/getCalEventResponses.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/features/bookings/lib/getCalEventResponses.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
⏰ 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). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (3)
packages/features/bookings/lib/getCalEventResponses.ts (3)
13-13: Good: scoped sub-logger with clear prefix.Helps grep-ability and isolates noise.
74-76: Use strict equality and clear guests when seats are enabled
Replaced==with===. Clearinggueststo[]whenseatsEnabledis true aligns with the UI’s behavior of disabling the guests field and won’t regress other flows.
9-9: No action needed; default import is correct.
packages/lib/logger.tsusesexport default logger, so importing withimport logger from "@calcom/lib/logger"matches the module signature and won’t cause a runtime mismatch.
| const backwardCompatibleResponses = | ||
| responses ?? (booking ? getBookingWithResponses(booking).responses : null); | ||
| if (!backwardCompatibleResponses) throw new Error("Couldn't get responses"); |
There was a problem hiding this comment.
Use HttpError and add context; avoid mutating the source by cloning.
Throwing a bare Error is inconsistent with the rest of this function and loses context. Also clone to avoid mutating inputs.
Apply:
- const backwardCompatibleResponses =
- responses ?? (booking ? getBookingWithResponses(booking).responses : null);
- if (!backwardCompatibleResponses) throw new Error("Couldn't get responses");
+ const baseResponses =
+ responses ?? (booking ? getBookingWithResponses(booking).responses : null);
+ if (!baseResponses) {
+ log.error("Could not derive booking responses", {
+ hasResponsesParam: !!responses,
+ hasBooking: !!booking,
+ });
+ throw new HttpError({ statusCode: 400, message: "Couldn't derive booking responses" });
+ }
+ const backwardCompatibleResponses = { ...baseResponses };Committable suggestion skipped: line range outside the PR's diff.
| if (!!!backwardCompatibleResponses.email) { | ||
| if (typeof backwardCompatibleResponses["attendeePhoneNumber"] !== "string") | ||
| if (typeof backwardCompatibleResponses["attendeePhoneNumber"] !== "string") { | ||
| log.error(`backwardCompatibleResponses: ${JSON.stringify(backwardCompatibleResponses)}`, { | ||
| responses, | ||
| bookingResponses: booking?.responses, | ||
| }); | ||
| throw new HttpError({ | ||
| statusCode: 400, | ||
| message: "Both Phone and Email are missing", | ||
| }); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce PII in logs and simplify triple-bang; improve error copy.
!!!xhurts readability; prefer!x.- Logging the full responses object and booking responses risks leaking PII. Log metadata only.
- Error message: use a clearer, action-oriented sentence.
Apply:
- if (!!!backwardCompatibleResponses.email) {
+ if (!backwardCompatibleResponses.email) {
if (typeof backwardCompatibleResponses["attendeePhoneNumber"] !== "string") {
- log.error(`backwardCompatibleResponses: ${JSON.stringify(backwardCompatibleResponses)}`, {
- responses,
- bookingResponses: booking?.responses,
- });
- throw new HttpError({
- statusCode: 400,
- message: "Both Phone and Email are missing",
- });
+ log.error("Missing both phone and email in booking responses", {
+ keys: Object.keys(backwardCompatibleResponses),
+ attendeePhoneNumberType: typeof backwardCompatibleResponses["attendeePhoneNumber"],
+ hasResponsesParam: !!responses,
+ hasBookingResponses: !!booking?.responses,
+ });
+ throw new HttpError({ statusCode: 400, message: "Either phone or email is required." });
}📝 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.
| if (!!!backwardCompatibleResponses.email) { | |
| if (typeof backwardCompatibleResponses["attendeePhoneNumber"] !== "string") | |
| if (typeof backwardCompatibleResponses["attendeePhoneNumber"] !== "string") { | |
| log.error(`backwardCompatibleResponses: ${JSON.stringify(backwardCompatibleResponses)}`, { | |
| responses, | |
| bookingResponses: booking?.responses, | |
| }); | |
| throw new HttpError({ | |
| statusCode: 400, | |
| message: "Both Phone and Email are missing", | |
| }); | |
| } | |
| if (!backwardCompatibleResponses.email) { | |
| if (typeof backwardCompatibleResponses["attendeePhoneNumber"] !== "string") { | |
| log.error("Missing both phone and email in booking responses", { | |
| keys: Object.keys(backwardCompatibleResponses), | |
| attendeePhoneNumberType: typeof backwardCompatibleResponses["attendeePhoneNumber"], | |
| hasResponsesParam: !!responses, | |
| hasBookingResponses: !!booking?.responses, | |
| }); | |
| throw new HttpError({ statusCode: 400, message: "Either phone or email is required." }); | |
| } |
🤖 Prompt for AI Agents
In packages/features/bookings/lib/getCalEventResponses.ts around lines 50–60,
simplify the visibility check by replacing the confusing triple-bang with a
single negation (use !backwardCompatibleResponses.email), stop logging full
response objects to avoid PII leaks (log only non-PII metadata such as
booking?.id, responses?.length, and whether attendeePhoneNumber is present), and
update the thrown error message to a clear, action-oriented sentence like
"Please provide either a valid email or a phone number." Ensure the log and
error changes are used in the same conditional branch where the phone/email
validation occurs.
E2E results are ready! |
4 participants
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?