feat: ✨ sendpayload logic in unified webhook service class #22895
Conversation
|
@shaun-ak is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR centralizes webhook dispatch across the codebase by introducing WebhookService.sendWebhook and replacing direct calls to getWebhooks/sendPayload in multiple handlers. Call sites now build subscriber options and payloads, then delegate delivery to WebhookService.sendWebhook. A new static sendWebhook method (and related init usage) was added to WebhookService. One function signature changed: cancelAttendeeSeat no longer accepts an incoming webhooks array. Tests for WebhookService were expanded to validate dispatch, error handling, concurrency, and edge cases. Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/05/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (08/05/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/lib/daily-webhook/triggerWebhooks.ts(6 hunks)packages/features/bookings/lib/handleBookingRequested.ts(2 hunks)packages/features/bookings/lib/handleCancelBooking.ts(5 hunks)packages/features/bookings/lib/handleConfirmation.ts(4 hunks)packages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts(3 hunks)packages/features/bookings/lib/handleWebhookTrigger.ts(2 hunks)packages/features/webhooks/lib/WebhookService.ts(2 hunks)packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts(2 hunks)packages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.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/handleCancelBooking.tspackages/features/webhooks/lib/WebhookService.tspackages/features/bookings/lib/handleBookingRequested.tspackages/trpc/server/routers/viewer/bookings/requestReschedule.handler.tspackages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.tsapps/web/lib/daily-webhook/triggerWebhooks.tspackages/features/bookings/lib/handleWebhookTrigger.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.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/handleCancelBooking.tspackages/features/webhooks/lib/WebhookService.tspackages/features/bookings/lib/handleBookingRequested.tspackages/trpc/server/routers/viewer/bookings/requestReschedule.handler.tspackages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.tsapps/web/lib/daily-webhook/triggerWebhooks.tspackages/features/bookings/lib/handleWebhookTrigger.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts
**/*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/features/webhooks/lib/WebhookService.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: For large pull requests (>500 lines changed or >10 files touched), advise splitting into smaller, focused PRs by feature, layer, dependency chain, or file/module
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/features/bookings/lib/handleCancelBooking.tspackages/features/webhooks/lib/WebhookService.tspackages/features/bookings/lib/handleBookingRequested.tspackages/trpc/server/routers/viewer/bookings/requestReschedule.handler.tspackages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.tsapps/web/lib/daily-webhook/triggerWebhooks.tspackages/features/bookings/lib/handleWebhookTrigger.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts
📚 Learning: for microsoft graph webhook handlers, when dealing with internal configuration errors (like missing ...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:50-60
Timestamp: 2025-07-18T17:54:04.613Z
Learning: For Microsoft Graph webhook handlers, when dealing with internal configuration errors (like missing MICROSOFT_WEBHOOK_TOKEN), it's better to return 200 OK with errors tracked in the response body rather than throwing 5xx errors. This prevents retry storms from Microsoft Graph and maintains webhook subscription health, while still providing visibility through error logs and structured error responses.
Applied to files:
packages/features/bookings/lib/handleBookingRequested.tspackages/features/bookings/lib/handleConfirmation.ts
📚 Learning: in packages/app-store/office365calendar/lib/calendarservice.ts, the fetcher method in office365calen...
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/bookings/requestReschedule.handler.tspackages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.tsapps/web/lib/daily-webhook/triggerWebhooks.tspackages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts
📚 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/handleSeats/cancel/cancelAttendeeSeat.ts
🧬 Code Graph Analysis (6)
packages/features/bookings/lib/handleCancelBooking.ts (1)
packages/features/webhooks/lib/WebhookService.ts (1)
WebhookService(12-43)
packages/features/webhooks/lib/WebhookService.ts (2)
packages/features/webhooks/lib/getWebhooks.ts (1)
GetSubscriberOptions(7-14)packages/features/webhooks/lib/sendPayload.ts (1)
WebhookPayloadType(96-96)
packages/features/bookings/lib/handleBookingRequested.ts (1)
packages/features/webhooks/lib/WebhookService.ts (1)
WebhookService(12-43)
packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (1)
packages/features/webhooks/lib/WebhookService.ts (1)
WebhookService(12-43)
packages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.ts (1)
packages/features/webhooks/lib/WebhookService.ts (1)
WebhookService(12-43)
packages/features/bookings/lib/handleConfirmation.ts (1)
packages/features/webhooks/lib/WebhookService.ts (1)
WebhookService(12-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (16)
packages/features/webhooks/lib/WebhookService.ts (1)
39-42: LGTM! Clean implementation of the centralized webhook dispatch method.The new static
sendWebhookmethod successfully consolidates webhook dispatch logic into a single entry point, aligning with the PR objectives. The implementation properly leverages existing initialization and payload sending mechanisms.packages/features/bookings/lib/handleBookingRequested.ts (1)
62-68: LGTM! Successfully refactored to use centralized webhook dispatch.The refactor correctly replaces the manual webhook fetching and sending pattern with the new
WebhookService.sendWebhookmethod, maintaining the same error handling behavior while reducing code complexity.packages/trpc/server/routers/viewer/ooo/outOfOfficeCreateOrUpdate.handler.ts (1)
317-359: LGTM! Clean refactor to centralized webhook service.The webhook dispatch has been properly refactored to use
WebhookService.sendWebhook. The subscriber options and payload construction are correctly maintained, achieving the centralization objective.packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (1)
326-333: LGTM! Webhook dispatch successfully centralized.The refactor correctly implements the centralized webhook dispatch pattern using
WebhookService.sendWebhook, maintaining all necessary subscriber options while simplifying the code.apps/web/lib/daily-webhook/triggerWebhooks.ts (2)
19-33: LGTM! Function correctly adapted to return subscriber options.The
getWebhooksByEventTriggerfunction has been properly refactored to return subscriber options instead of fetching webhooks directly, aligning with the new centralized webhook dispatch pattern.
45-59: LGTM! Webhook triggering properly centralized.Both
triggerRecordingReadyWebhookand the related functions have been successfully refactored to use the centralizedWebhookService.sendWebhookmethod.packages/features/bookings/lib/handleCancelBooking.ts (3)
10-10: Import changes look goodThe imports are correctly updated to use the centralized
WebhookServiceand necessary types.Also applies to: 42-42
289-289: Simplified dataForWebhooks structureGood simplification - removing the
webhooksarray fromdataForWebhookssince webhook management is now handled internally byWebhookService.
309-315: Excellent webhook dispatch consolidationThe refactor to use
WebhookService.sendWebhookproperly centralizes webhook dispatch logic and error handling, improving maintainability and consistency across the codebase.packages/features/bookings/lib/handleWebhookTrigger.ts (2)
2-5: Import updates align with centralized webhook serviceThe imports correctly reflect the transition to using
WebhookServicefor webhook dispatch.
15-16: Clean webhook trigger implementationThe simplified implementation using
WebhookService.sendWebhookmaintains functionality while improving code clarity and consistency.packages/features/bookings/lib/handleConfirmation.ts (2)
11-15: Import updates support partial refactorThe imports correctly support the partial refactor where immediate webhooks use
WebhookServicewhile scheduled triggers still needgetWebhooks.
470-477: BOOKING_CREATED webhook properly refactoredThe webhook dispatch for
BOOKING_CREATEDcorrectly uses the centralizedWebhookService.sendWebhookmethod.packages/features/bookings/lib/handleSeats/cancel/cancelAttendeeSeat.ts (3)
3-5: Import updates support webhook refactorThe imports correctly add
WebhookServiceand the helper function needed for resolving organization IDs.
37-37: Correct destructuring of webhook dataThe destructuring properly extracts the required data from the simplified
dataForWebhooksstructure.
149-165: Well-implemented webhook dispatch with proper error handlingThe webhook sending implementation correctly:
- Resolves the organization ID before dispatch
- Uses the centralized
WebhookService.sendWebhookmethod- Includes proper error handling that logs failures without interrupting the cancellation flow
There was a problem hiding this comment.
hey @shaun-ak thanks for the pr. Could you please Add tests to verify webhook timing and error scenarios ?
|
Hey @Devanshusharma2005 , I have added test cases, commit |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/features/webhooks/lib/WebhookService.test.ts (2)
56-60: Nit: Fix comment wording and avoid unnecessary await on a sync getter
init()being async is the reason for awaiting the initialization (not an IIFE).getWebhooks()is synchronous; no need to await it.Apply this diff:
- // Has to be called with await due to the iffi being async + // init() is async - expect(await service.getWebhooks()).toEqual(mockWebhooks); + expect(service.getWebhooks()).toEqual(mockWebhooks);
64-121: Strengthen assertions: validatecreatedAtis ISO-8601Right now we only assert a string. Tighten it to an ISO-8601 match to ensure the timestamp is well-formed.
Apply this diff:
@@ - expect(sendOrSchedulePayload).toHaveBeenCalledTimes(mockWebhooks.length); - - mockWebhooks.forEach((webhook) => { + expect(sendOrSchedulePayload).toHaveBeenCalledTimes(mockWebhooks.length); + + const iso8601 = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d{3})?Z$/; + mockWebhooks.forEach((webhook) => { expect(sendOrSchedulePayload).toHaveBeenCalledWith( webhook.secret, mockOptions.triggerEvent, - expect.any(String), + expect.stringMatching(iso8601), webhook, payload ); });
📜 Review details
Configuration used: CodeRabbit UI
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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/webhooks/lib/WebhookService.test.ts(3 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/webhooks/lib/WebhookService.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/features/webhooks/lib/WebhookService.test.ts
🧬 Code Graph Analysis (1)
packages/features/webhooks/lib/WebhookService.test.ts (1)
packages/features/webhooks/lib/WebhookService.ts (2)
getWebhooks(18-20)WebhookService(12-43)
⏰ Context from checks skipped due to timeout of 90000ms. 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 (7)
packages/features/webhooks/lib/WebhookService.test.ts (7)
8-12: Mocks for send/lookup wiring look correctImporting and mocking
sendOrSchedulePayloadalongsidegetWebhooksis the right approach to isolate the unit under test.
161-221: Good coverage for partial failuresNice validation that we dispatch to all webhooks even when some fail.
222-244: Correct handling for no subscribersAsserting no calls when there are no webhooks is spot on.
304-348: Null secret scenario coveredGood to see we accept null secrets and pass them through as-is to the dispatcher.
349-393: Undefined appId scenario coveredLooks good; ensures we don’t accidentally coerce/omit fields unintentionally.
394-438: Null payloadTemplate scenario coveredThis test guards an easy-to-miss edge case. Good.
439-481: Static sendWebhook API test is solidVerifies both subscriber lookup and dispatch arguments. Nice.
|
Hey @shaun-ak Thank you for your PR. The issue this PR is trying to resolve has been updated with a lot more changes and expectations, and there is another PR already trying to tackle it (which just needs to be broken down into separate small chunks for easier reviewing and targeted update). I sincerely appreciate your efforts and thank you for your contribution. I also apologize for having to close this down, however, there is no alternative as it isn't sensible to go this route right now when we have a much larger change coming in soon after. I hope this makes sense 🙏 |
|
Really appreciate the detailed feedback and clarity you provided @alishaz-polymath 🙏 |
What does this PR do?
Video--
https://cap.so/s/wvwksvbh112eab1
Mandatory Tasks (DO NOT REMOVE)