feat: Booking EmailAndSms Notifications Tasker #24944
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
keithwillcode
added
core
ThyMinimalDev
force-pushed
the
async-tasker-booking-email-and-sms
branch
from
e9af47d to
4db7cd9
Compare
ThyMinimalDev
force-pushed
the
async-tasker-booking-email-and-sms
branch
from
4db7cd9 to
dc59968
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
ThyMinimalDev
commented
Nov 5, 2025
ThyMinimalDev
commented
Nov 5, 2025
hbjORbj
reviewed
Nov 6, 2025
Contributor
hbjORbj
left a comment
hbjORbj
left a comment
There was a problem hiding this comment.
Code looks great, tests and typechecks need to be fixed though. Will come back later for a 2nd review and some testing.
…ests The unit tests were failing because BookingEmailAndSmsTriggerTasker.ts had static imports of trigger files that depend on @trigger.dev/sdk. This caused Vitest to try to resolve the SDK at module load time, even though it should be optional. Changed all imports in BookingEmailAndSmsTriggerTasker.ts from static to dynamic (using await import()) so the trigger files are only loaded when the tasker methods are actually called, not at module load time during tests. This fixes the 'Failed to load url @trigger.dev/sdk' errors that were causing 28+ test failures. Co-Authored-By: morgan@cal.com <morgan@cal.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 56 files
Prompt for AI agents (all 8 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/bookings/lib/tasker/BookingEmailAndSmsTasker.ts">
<violation number="1" location="packages/features/bookings/lib/tasker/BookingEmailAndSmsTasker.ts:33">
Early return bypasses try-catch error handling and logging. Unlike other actions, `rrReschedule` failures won't be logged and errors won't be caught. Consider assigning to `taskResponse` instead and letting the flow continue to the logging.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:45">
Script naming inconsistency: should be `deploy:trigger` to match PR documentation and the underlying `@calcom/features` script name. Currently named `dev:deploy` which doesn't follow the `*:trigger` naming pattern.</violation>
</file>
<file name="packages/features/bookings/lib/tasker/BookingEmailAndSmsTaskService.ts">
<violation number="1" location="packages/features/bookings/lib/tasker/BookingEmailAndSmsTaskService.ts:137">
Hardcoding `changedOrganizer: true` and `isRescheduledByBooker: true` will cause incorrect email notifications. These values should reflect actual reschedule context - whether the organizer changed and who initiated the reschedule. Consider extending the payload type to include these fields or deriving them from booking data.</violation>
</file>
<file name="apps/api/v2/src/lib/logger.bridge.ts">
<violation number="1" location="apps/api/v2/src/lib/logger.bridge.ts:9">
Typo from search-and-replace: "Add unknown other settings" should be "Add any other settings". The word "any" here was English (meaning "additional"), not the TypeScript type.</violation>
</file>
<file name="packages/lib/tasker/Tasker.ts">
<violation number="1" location="packages/lib/tasker/Tasker.ts:19">
Logical error in warning condition: uses `&&` instead of `||`. When only ONE env var is missing, the async tasker falls back to sync mode silently without logging a warning. This should use `||` to warn when EITHER env var is missing.</violation>
</file>
<file name="packages/features/package.json">
<violation number="1" location="packages/features/package.json:11">
Scripts use `npx trigger.dev@latest` which ignores the local devDependency and may cause version drift. Consider using `npx trigger.dev` (without `@latest`) to use the locally installed version for reproducible builds, or remove the devDependency if always using latest is intentional.</violation>
</file>
<file name="turbo.json">
<violation number="1" location="turbo.json:461">
The `dev:trigger` and `deploy:trigger` tasks are missing `cache: false`. Following the established pattern in this codebase (see `dev`, `deploy`, and `dx` tasks), development and deployment tasks should disable caching to ensure the command actually executes each time rather than returning cached results.</violation>
</file>
<file name="packages/features/bookings/repositories/BookingRepository.ts">
<violation number="1" location="packages/features/bookings/repositories/BookingRepository.ts:1315">
Using `steps: true` fetches all WorkflowStep fields including potentially sensitive data like `reminderBody`, `emailSubject`, `sendTo`, and `agentId`. Per project Prisma guidelines, explicitly select only the fields needed for the CalEvent builder instead of fetching the entire relation.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
packages/features/bookings/lib/tasker/BookingEmailAndSmsTasker.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/tasker/BookingEmailAndSmsTaskService.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
4 issues found across 56 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/bookings/di/BookingEmailSmsHandler.module.ts">
<violation number="1" location="packages/features/bookings/di/BookingEmailSmsHandler.module.ts:26">
Unused import and incorrect type export. `RegularBookingService` is imported but never used in the module's logic. Based on the pattern in other `.module.ts` files (e.g., `BookingCancelService.module.ts`, `RecurringBookingService.module.ts`), this should export the type of the class being bound (`BookingEmailSmsHandler`), not an unrelated service.</violation>
</file>
<file name="packages/features/bookings/di/tasker/BookingEmailAndSmsTriggerDevTasker.container.ts">
<violation number="1" location="packages/features/bookings/di/tasker/BookingEmailAndSmsTriggerDevTasker.container.ts:8">
Function name mismatch: `getBookingEmailAndSmsSyncTasker` should be `getBookingEmailAndSmsTriggerDevTasker` to match the return type and differentiate from the actual Sync tasker container. This appears to be a copy-paste error that will cause confusion and potential naming collisions when both containers are imported.</violation>
</file>
<file name="packages/lib/tasker/Tasker.ts">
<violation number="1" location="packages/lib/tasker/Tasker.ts:2">
Static import of `@trigger.dev/sdk` will load the SDK even when async tasking is disabled. Consider using dynamic import to avoid loading the SDK during tests or when `ENABLE_ASYNC_TASKER` is false, consistent with the PR's stated approach for other Trigger.dev imports.</violation>
<violation number="2" location="packages/lib/tasker/Tasker.ts:41">
Rule violated: **Avoid Logging Sensitive Information**
Logging `{ args }` will expose PII (emails, phone numbers, booking details) in logs. Since this is a booking email/SMS tasker, the args will contain attendee contact information. Consider removing `args` from the log statement or redacting sensitive fields before logging.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
packages/features/bookings/di/tasker/BookingEmailAndSmsTriggerDevTasker.container.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,79 @@ | |||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
| import { configure } from "@trigger.dev/sdk"; | |||
Contributor
There was a problem hiding this comment.
Static import of @trigger.dev/sdk will load the SDK even when async tasking is disabled. Consider using dynamic import to avoid loading the SDK during tests or when ENABLE_ASYNC_TASKER is false, consistent with the PR's stated approach for other Trigger.dev imports.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/lib/tasker/Tasker.ts, line 2:
<comment>Static import of `@trigger.dev/sdk` will load the SDK even when async tasking is disabled. Consider using dynamic import to avoid loading the SDK during tests or when `ENABLE_ASYNC_TASKER` is false, consistent with the PR's stated approach for other Trigger.dev imports.</comment>
<file context>
@@ -0,0 +1,79 @@
+/* eslint-disable @typescript-eslint/no-explicit-any */
+import { configure } from "@trigger.dev/sdk";
+
+import { ENABLE_ASYNC_TASKER } from "../constants";
</file context>
10 tasks
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/platform/atoms/vite.config.ts">
<violation number="1" location="packages/platform/atoms/vite.config.ts:73">
Standalone comma creates an empty array slot in the externals array. This is likely unintentional and should be removed.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| "@prisma/client", | ||
| "react/jsx-dev-runtime", | ||
| , "react-awesome-query-builder", "react-awesome-query-builder", "react-awesome-query-builder"], | ||
| , |
Contributor
There was a problem hiding this comment.
Standalone comma creates an empty array slot in the externals array. This is likely unintentional and should be removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform/atoms/vite.config.ts, line 73:
<comment>Standalone comma creates an empty array slot in the externals array. This is likely unintentional and should be removed.</comment>
<file context>
@@ -71,7 +70,12 @@ export default defineConfig(({ mode }) => {
"@prisma/client",
"react/jsx-dev-runtime",
- , "react-awesome-query-builder", "react-awesome-query-builder", "react-awesome-query-builder"],
+ ,
+ "react-awesome-query-builder",
+ "react-awesome-query-builder",
</file context>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds async email and SMS sending for booking events using Trigger.dev with a safe sync fallback to reduce request time and improve reliability. Integrates the tasker into RegularBookingService via DI, aligns logging with tslog, and loads Trigger.dev tasks dynamically to avoid test failures.
Mandatory Tasks (DO NOT REMOVE)
Summary by cubic
Adds async email and SMS sending for booking events using Trigger.dev with a safe sync fallback to reduce request time and improve reliability. Integrates the tasker into RegularBookingService via DI, aligns logging with tslog, and loads Trigger.dev tasks dynamically to avoid test failures.
New Features
Migration
Written for commit 974fb49. Summary will update automatically on new commits.