chore: Add logging to handleNewBooking #22868
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change enhances logging and logger configuration across three files. In the booking creation logic ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/01/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/02/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/features/bookings/lib/handleNewBooking.ts(1 hunks)packages/lib/getBusyTimes.ts(1 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/features/bookings/lib/handleNewBooking.tspackages/lib/getBusyTimes.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/handleNewBooking.tspackages/lib/getBusyTimes.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
📚 Learning: the outlook calendar integration in cal.com intentionally reuses subscription ids across multiple ev...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
📚 Learning: applies to **/*.{ts,tsx} : flag excessive day.js use in performance-critical code; prefer native dat...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{ts,tsx} : Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js `.utc()` in hot paths like loops
Applied to files:
packages/lib/getBusyTimes.ts
| } else { | ||
| logger.warn(`No credentials found for user ${userId}`, { | ||
| selectedCalendarIds: selectedCalendars.map((calendar) => calendar.id), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log message incorrectly lumps two distinct cases together
The else branch fires both when (a) the user really has no credentials and when (b) bypassBusyCalendarTimes is true. Case (b) can still have credentials, so the warning “No credentials found” is misleading.
-} else {
- logger.warn(`No credentials found for user ${userId}`, {
- selectedCalendarIds: selectedCalendars.map((calendar) => calendar.id),
- });
+} else if (credentials?.length === 0) {
+ logger.warn(`No credentials found for user ${userId}`, {
+ selectedCalendarIds: selectedCalendars.map((c) => c.id),
+ });
+} else {
+ logger.debug(`Busy-calendar times bypassed for user ${userId}`, {
+ bypassBusyCalendarTimes,
+ selectedCalendarIds: selectedCalendars.map((c) => c.id),
+ });
}📝 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.
| } else { | |
| logger.warn(`No credentials found for user ${userId}`, { | |
| selectedCalendarIds: selectedCalendars.map((calendar) => calendar.id), | |
| }); | |
| } else if (credentials?.length === 0) { | |
| logger.warn(`No credentials found for user ${userId}`, { | |
| selectedCalendarIds: selectedCalendars.map((c) => c.id), | |
| }); | |
| } else { | |
| logger.debug(`Busy-calendar times bypassed for user ${userId}`, { | |
| bypassBusyCalendarTimes, | |
| selectedCalendarIds: selectedCalendars.map((c) => c.id), | |
| }); |
🤖 Prompt for AI Agents
In packages/lib/getBusyTimes.ts around lines 255 to 258, the else branch logs a
warning message "No credentials found" that incorrectly applies both when the
user truly has no credentials and when bypassBusyCalendarTimes is true, which
may still have credentials. To fix this, separate the conditions so that the
warning about missing credentials only logs when the user actually lacks
credentials, and handle the bypassBusyCalendarTimes case distinctly without
misleading warnings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
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