Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Rate limit exceeded@hariombalhara has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (21)
WalkthroughAdds feature-flagged booking email validation: new EmailValidationService (cache-first Cal.com flow and provider validation with timeout/fallback), ZeroBounce provider implementation, DTOs/interfaces, Redis caching keys/TTL and schema, DI tokens/modules/loaders and a container accessor, and a Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
from
8f8dcbd to
7e667c2
Compare
There was a problem hiding this comment.
Not focussing on fixing eslint errors for test files as this is a bit urgent PR.
|
|
||
| export function getEmailValidationService(): IEmailValidationService { | ||
| emailValidationCachingProxyServiceModule.loadModule(container); | ||
| return container.get<IEmailValidationService>(DI_TOKENS.EMAIL_VALIDATION_CACHING_PROXY_SERVICE); |
There was a problem hiding this comment.
Injecting the caching proxy instead of the actual service
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
from
7e667c2 to
a767382
Compare
E2E results are ready! |
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
2 times, most recently
from
f531694 to
d8ec66b
Compare
packages/features/emailValidation/lib/service/EmailValidationCachingProxy.ts
Outdated
Show resolved
Hide resolved
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
from
d8ec66b to
55f8c01
Compare
dosubot
bot
added
bookings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/features/redis/di/redisModule.ts (1)
19-25: Consider aligning return type with other module loaders.The
loadModulefunction returnsredisModule, but other module loaders in the codebase (e.g.,Cache.ts,prisma.module.ts) don't return a value. While returning the module may be convenient for chaining, it creates inconsistency.For consistency, consider removing the return statement:
export const moduleLoader: ModuleLoader = { token, loadModule: (container: Container) => { container.load(moduleToken, redisModule); - return redisModule; }, };Alternatively, if the return is intentional for a specific use case, update other module loaders to match this pattern and document the rationale.
📜 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 (24)
apps/web/test/utils/bookingScenario/bookingScenario.ts(17 hunks)packages/features/bookings/lib/dto/types.d.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(3 hunks)packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts(1 hunks)packages/features/di/tokens.ts(2 hunks)packages/features/emailValidation/di/EmailValidation.container.ts(1 hunks)packages/features/emailValidation/di/EmailValidationCachingProxy.module.ts(1 hunks)packages/features/emailValidation/di/EmailValidationProviderService.module.ts(1 hunks)packages/features/emailValidation/di/EmailValidationService.module.ts(1 hunks)packages/features/emailValidation/lib/dto/types.d.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationCachingProxy.test.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationCachingProxy.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.test.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts(1 hunks)packages/features/emailValidation/lib/validateBookingEmail.ts(1 hunks)packages/features/flags/config.ts(1 hunks)packages/features/flags/hooks/index.ts(1 hunks)packages/features/redis/di/redisModule.ts(1 hunks)packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/flags/config.tspackages/features/emailValidation/lib/dto/types.d.tspackages/features/emailValidation/di/EmailValidation.container.tspackages/features/emailValidation/lib/validateBookingEmail.tspackages/features/emailValidation/lib/service/EmailValidationCachingProxy.test.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tspackages/features/emailValidation/di/EmailValidationCachingProxy.module.tspackages/features/emailValidation/di/EmailValidationProviderService.module.tspackages/features/redis/di/redisModule.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/bookings/lib/dto/types.d.tspackages/features/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tspackages/features/flags/hooks/index.tspackages/features/emailValidation/lib/service/EmailValidationCachingProxy.tspackages/features/di/tokens.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.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/flags/config.tspackages/features/emailValidation/lib/dto/types.d.tspackages/features/emailValidation/di/EmailValidation.container.tspackages/features/emailValidation/lib/validateBookingEmail.tspackages/features/emailValidation/lib/service/EmailValidationCachingProxy.test.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tspackages/features/emailValidation/di/EmailValidationCachingProxy.module.tspackages/features/emailValidation/di/EmailValidationProviderService.module.tspackages/features/redis/di/redisModule.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/bookings/lib/dto/types.d.tspackages/features/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tspackages/features/flags/hooks/index.tspackages/features/emailValidation/lib/service/EmailValidationCachingProxy.tspackages/features/di/tokens.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.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/flags/config.tspackages/features/emailValidation/lib/dto/types.d.tspackages/features/emailValidation/di/EmailValidation.container.tspackages/features/emailValidation/lib/validateBookingEmail.tspackages/features/emailValidation/lib/service/EmailValidationCachingProxy.test.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tspackages/features/emailValidation/di/EmailValidationCachingProxy.module.tspackages/features/emailValidation/di/EmailValidationProviderService.module.tspackages/features/redis/di/redisModule.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/bookings/lib/dto/types.d.tspackages/features/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tspackages/features/flags/hooks/index.tspackages/features/emailValidation/lib/service/EmailValidationCachingProxy.tspackages/features/di/tokens.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.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/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts
🧠 Learnings (1)
📚 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:
apps/web/test/utils/bookingScenario/bookingScenario.ts
🧬 Code graph analysis (16)
packages/features/emailValidation/di/EmailValidation.container.ts (2)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
IEmailValidationCachingProxy(8-13)packages/features/di/tokens.ts (1)
DI_TOKENS(3-65)
packages/features/emailValidation/lib/validateBookingEmail.ts (1)
packages/features/emailValidation/di/EmailValidation.container.ts (1)
getEmailValidationService(9-12)
packages/features/emailValidation/lib/service/EmailValidationCachingProxy.test.ts (3)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
IEmailValidationService(3-6)packages/features/emailValidation/lib/service/EmailValidationCachingProxy.ts (1)
EmailValidationCachingProxy(26-115)packages/features/emailValidation/lib/dto/types.d.ts (1)
EmailValidationResult(6-16)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
packages/features/emailValidation/lib/dto/types.d.ts (3)
EmailValidationRequest(1-4)EmailValidationResult(6-16)EmailValidationStatus(18-18)
packages/features/emailValidation/di/EmailValidationCachingProxy.module.ts (5)
packages/features/di/di.ts (2)
bindModuleToClassOnToken(57-113)ModuleLoader(4-4)packages/features/di/tokens.ts (1)
DI_TOKENS(3-65)packages/features/emailValidation/lib/service/EmailValidationCachingProxy.ts (1)
EmailValidationCachingProxy(26-115)packages/features/emailValidation/di/EmailValidationService.module.ts (2)
emailValidationServiceLoader(13-22)moduleLoader(24-27)packages/features/redis/di/redisModule.ts (1)
moduleLoader(19-25)
packages/features/emailValidation/di/EmailValidationProviderService.module.ts (3)
packages/features/di/di.ts (2)
bindModuleToClassOnToken(57-113)ModuleLoader(4-4)packages/features/di/tokens.ts (1)
DI_TOKENS(3-65)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
ZeroBounceEmailValidationProviderService(17-131)
packages/features/redis/di/redisModule.ts (4)
packages/features/di/di.ts (2)
ModuleLoader(4-4)Container(114-114)packages/features/di/tokens.ts (1)
DI_TOKENS(3-65)packages/features/di/modules/Cache.ts (1)
moduleLoader(14-19)packages/prisma/prisma.module.ts (1)
moduleLoader(13-19)
packages/features/emailValidation/lib/service/EmailValidationService.ts (3)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
IEmailValidationService(3-6)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(11-14)packages/features/emailValidation/lib/dto/types.d.ts (3)
EmailValidationRequest(1-4)EmailValidationResult(6-16)EmailValidationStatus(18-18)
packages/features/emailValidation/di/EmailValidationService.module.ts (4)
packages/features/di/di.ts (2)
bindModuleToClassOnToken(57-113)ModuleLoader(4-4)packages/features/di/tokens.ts (1)
DI_TOKENS(3-65)packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
EmailValidationService(20-115)packages/features/emailValidation/di/EmailValidationProviderService.module.ts (2)
emailValidationProviderServiceLoader(11-17)moduleLoader(19-22)
packages/features/emailValidation/lib/service/EmailValidationService.test.ts (3)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(11-14)packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
EmailValidationService(20-115)packages/features/emailValidation/lib/dto/types.d.ts (2)
EmailValidationRequest(1-4)EmailValidationResult(6-16)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts (1)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
ZeroBounceEmailValidationProviderService(17-131)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (2)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(11-14)packages/features/emailValidation/lib/dto/types.d.ts (3)
EmailValidationStatus(18-18)EmailValidationRequest(1-4)EmailValidationResult(6-16)
packages/features/emailValidation/lib/service/EmailValidationCachingProxy.ts (3)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (2)
IEmailValidationCachingProxy(8-13)IEmailValidationService(3-6)packages/features/emailValidation/lib/dto/types.d.ts (3)
EmailValidationStatus(18-18)EmailValidationRequest(1-4)EmailValidationResult(6-16)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
packages/features/emailValidation/lib/dto/types.d.ts (3)
EmailValidationRequest(1-4)EmailValidationResult(6-16)EmailValidationStatus(18-18)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/features/emailValidation/lib/validateBookingEmail.ts (1)
validateBookingEmailGenerator(21-120)
packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts (4)
packages/features/redis/RedisService.ts (1)
RedisService(5-50)apps/web/test/utils/bookingScenario/bookingScenario.ts (8)
enableEmailValidationForTeam(1690-1719)getBooker(2267-2281)getOrganizer(1530-1589)TestData(1249-1521)getGoogleCalendarCredential(1202-1210)createBookingScenario(987-1018)getScenarioData(1591-1677)mockCalendarToHaveNoBusySlots(2012-2025)packages/features/bookings/lib/handleNewBooking/test/getNewBookingHandler.ts (1)
getNewBookingHandler(6-8)apps/web/test/utils/bookingScenario/expects.ts (2)
expectBookingToBeInDatabase(418-435)expectSuccessfulBookingCreationEmails(459-645)
🔇 Additional comments (7)
packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql (1)
1-9: LGTM! Well-structured migration.The migration correctly implements an idempotent feature flag insertion with a safe default (disabled). The comprehensive description clearly explains the feature's purpose and blocking criteria.
packages/features/flags/config.ts (1)
31-31: LGTM! Flag properly integrated.The new flag follows the existing naming convention and type pattern, correctly extending both AppFlags and TeamFeatures types.
packages/features/flags/hooks/index.ts (1)
30-30: LGTM! Default value consistent with migration.The flag is correctly initialized to
false, matching the database migration default and ensuring safe rollout behavior.packages/features/bookings/lib/dto/types.d.ts (1)
37-37: LGTM! Field properly typed.The optional
clientIPfield is correctly typed and positioned, supporting the email validation feature's need for IP-based context without breaking existing code paths.turbo.json (1)
288-289: LGTM! Environment variable properly configured.The
ZEROBOUNCE_API_KEYis correctly added toglobalEnv, making it available across the monorepo for the email validation provider service.packages/features/di/tokens.ts (1)
8-8: LGTM! DI tokens properly defined.All new tokens follow the established naming convention and correctly pair service tokens with their corresponding module tokens. The naming is clear and aligns with the DI modules introduced in this PR.
Also applies to: 57-62
packages/features/emailValidation/di/EmailValidation.container.ts (1)
9-12: Confirm container.load idempotency in getEmailValidationService
The call toemailValidationCachingProxyServiceModule.loadModule(container)runs on every invocation; verify that @evyweb/ioctopus’scontainer.loadis idempotent and low-overhead. If it isn’t, load the module once at startup or memoize this function.
packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts
Show resolved
Hide resolved
packages/features/emailValidation/lib/service/EmailValidationCachingProxy.ts
Outdated
Show resolved
Hide resolved
packages/features/emailValidation/lib/service/EmailValidationService.test.ts
Outdated
Show resolved
Hide resolved
Udit-takkar
left a comment
Udit-takkar
left a comment
There was a problem hiding this comment.
On cache miss, checks Cal.com database for verified users
@hariombalhara but any user can sign up on cal.com using temporary email address and verify that email
|
I think that could be a valid concern. Let's discuss internally |
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts
Show resolved
Hide resolved
- Update IEmailValidationProviderService interface to accept optional signal parameter - Pass signal through to fetch call in ZeroBounceEmailValidationProviderService - Wire signal from AbortController in EmailValidationService to provider This prevents network requests from continuing after timeout, avoiding resource leaks. Addresses review comment from Udit-takkar on PR #24196 Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
* feat: Pass abort signal to fetch in email validation provider - Update IEmailValidationProviderService interface to accept optional signal parameter - Pass signal through to fetch call in ZeroBounceEmailValidationProviderService - Wire signal from AbortController in EmailValidationService to provider This prevents network requests from continuing after timeout, avoiding resource leaks. Addresses review comment from Udit-takkar on PR #24196 Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com> * refactor: Rename signal to abortSignal for clarity - Rename parameter from 'signal' to 'abortSignal' to clearly convey it's for aborting fetch requests - Update interface, implementation, and call site - Update tests to match new object parameter syntax Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@Udit-takkar Abort signal handled now. Thanks to Devin !! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
68-76: PII exposure in error logs contradicts documented privacy requirements.The current logging includes full email addresses and raw API responses containing email data. While the maintainer noted this is needed for investigation, the PR objectives explicitly document: "PII protection via SHA-256 hashing for cache keys and removal of plaintext emails from logs."
Consider these options:
- Mask emails (first 10 chars +
***) and sanitizerawResponse.addressfield before logging- Document an exception for this rare error path and acknowledge the PII exposure risk
- Use a separate secure logging channel for full PII-containing error details
Based on PR objectives documenting PII protection requirements.
🧹 Nitpick comments (1)
packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
208-214: Verify error handling doesn't mask configuration issues.The catch-all error handler (lines 208-214) allows bookings to proceed on any provider validation failure. While this maintains availability, it could mask critical issues like misconfiguration or persistent provider outages. Consider:
- Differentiating between transient errors (network) vs permanent ones (config)
- Adding alerting for sustained provider failures
- Tracking error rates to detect systemic issues
Would you like me to generate a script to check if there's existing alerting/monitoring for provider validation failures?
📜 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 (4)
packages/features/emailValidation/lib/service/EmailValidationService.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*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/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tspackages/features/emailValidation/lib/service/EmailValidationService.ts
**/*.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/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/service/EmailValidationService.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/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/service/EmailValidationService.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/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/service/EmailValidationService.ts
🧠 Learnings (1)
📓 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.
🧬 Code graph analysis (4)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (2)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/emailValidation/lib/dto/types.ts (3)
EmailValidationRequest(3-6)EmailValidationResult(54-65)EmailValidationStatus(52-52)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
packages/features/emailValidation/lib/dto/types.ts (2)
EmailValidationRequest(3-6)EmailValidationResult(54-65)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts (1)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
ZeroBounceEmailValidationProviderService(17-120)
packages/features/emailValidation/lib/service/EmailValidationService.ts (5)
packages/lib/server/PiiHasher.ts (1)
hashEmail(18-22)packages/features/emailValidation/lib/dto/types.ts (4)
validationStatusSchema(13-50)EmailValidationStatus(52-52)EmailValidationResult(54-65)EmailValidationRequest(3-6)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (2)
IEmailValidationService(12-36)EmailValidationResponse(3-5)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (7)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
1-19: LGTM: Clean interface design with clear separation of concerns.The interface properly abstracts provider-specific validation logic while delegating blocking decisions to the service layer. The inclusion of
abortSignalfor timeout handling is a good practice.packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (2)
22-24: Constructor correctly handles API key initialization.The lazy validation approach (checking in
validateEmailrather than constructor) allows the service to be instantiated even without the key, which is appropriate for DI scenarios.
48-56: Abort signal is properly wired to fetch.The
abortSignalis correctly passed through to the fetch request, enabling timeout handling from the service layer.packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts (1)
1-109: Comprehensive test coverage for the provider service.The test suite appropriately covers successful validation, missing configuration, network failures, server errors, and IP address handling. The mocking strategy is sound.
packages/features/emailValidation/lib/service/EmailValidationService.ts (3)
67-92: Cache operations are resilient with appropriate error handling.The non-fatal error handling for cache operations ensures that validation can proceed even when Redis is unavailable, maintaining system reliability.
122-173: Validation flow correctly balances reliability and cost control.The conditional
continueWithProviderlogic (lines 143-148) prevents API hammering when cache is unavailable, which protects against cost explosion and rate limiting. The fallback behavior on DB errors (lines 169-170) is appropriately conservative.
220-275: Timeout implementation is robust with proper cleanup.The Promise.race pattern with AbortController correctly enforces the 3-second timeout. The timeout ID is properly cleared in both success (line 241) and error (line 254) paths, preventing leaks.
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
from
45f007d to
fe43010
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/features/bookings/lib/handleNewBooking.ts (1)
580-623: Await email validation before the early return.The early return at line 615 bypasses email validation. When
existingBookingis found, the function returns immediately without awaiting the provider validation promise started at line 508. If ZeroBounce flags the email as invalid/spam during the parallel check, this early return allows the booking to proceed anyway.Apply this fix to await validation before returning:
if (existingBooking) { + await emailValidationResult?.waitForProviderValidation(); + const hasPayments = existingBooking.payment.length > 0; const isPaidBooking = existingBooking.paid || !hasPayments; const shouldShowPaymentForm = requiresPayment && !isPaidBooking; const firstPayment = shouldShowPaymentForm ? existingBooking.payment[0] : undefined; const bookingResponse = { ...existingBooking, user: { ...existingBooking.user, email: null, }, paymentRequired: shouldShowPaymentForm, seatReferenceUid: "", }; return { ...bookingResponse, luckyUsers: bookingResponse.userId ? [bookingResponse.userId] : [], isDryRun, ...(isDryRun ? { troubleshooterData } : {}), paymentUid: firstPayment?.uid, paymentId: firstPayment?.id, }; }packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts (1)
289-301: Restore the base ZeroBounce fetch mock after the fallback override.The test replaces the global
fetchmock implementation but never restores the original ZeroBounce stub, so every subsequent test in the suite keeps hitting the "network error" branch. This creates test order-dependency and can cause false failures.Apply this fix to scope the override to this test only:
+ const fetchMock = vi.mocked(globalThis.fetch); + const originalImplementation = fetchMock.getMockImplementation(); + // Mock ZeroBounce API to fail with network error for this specific email // eslint-disable-next-line @typescript-eslint/no-explicit-any - (globalThis.fetch as any).mockImplementation((url: string | URL) => { + fetchMock.mockImplementation((url: string | URL) => { const urlString = url.toString(); if (urlString.includes("zerobounce.net/v2/validate")) { return Promise.reject(new Error("Network error: Failed to connect to ZeroBounce API")); } return Promise.resolve({ ok: false, status: 404, } as Response); }); mockCalendarToHaveNoBusySlots("googlecalendar", { create: { id: "google_calendar_event_id" }, }); const mockBookingData = getMockRequestDataForBooking({ data: { eventTypeId: 1, responses: { email: booker.email, name: booker.name, location: { optionValue: "", value: "New York" }, }, }, }); - // Should succeed because it falls back to allowing the booking when API fails - const createdBooking = await handleNewBooking({ - bookingData: mockBookingData, - }); + try { + // Should succeed because it falls back to allowing the booking when API fails + const createdBooking = await handleNewBooking({ + bookingData: mockBookingData, + }); - expect(createdBooking.responses).toMatchObject({ - name: booker.name, - email: booker.email, - }); + expect(createdBooking.responses).toMatchObject({ + name: booker.name, + email: booker.email, + }); - await expectBookingToBeInDatabase({ - description: "", - uid: createdBooking.uid!, - eventTypeId: mockBookingData.eventTypeId, - status: BookingStatus.ACCEPTED, - }); + await expectBookingToBeInDatabase({ + description: "", + uid: createdBooking.uid!, + eventTypeId: mockBookingData.eventTypeId, + status: BookingStatus.ACCEPTED, + }); - expectSuccessfulBookingCreationEmails({ - booking: { uid: createdBooking.uid! }, - booker, - organizer, - emails, - iCalUID: createdBooking.iCalUID, - }); + expectSuccessfulBookingCreationEmails({ + booking: { uid: createdBooking.uid! }, + booker, + organizer, + emails, + iCalUID: createdBooking.iCalUID, + }); + } finally { + if (originalImplementation) { + fetchMock.mockImplementation(originalImplementation); + } else { + fetchMock.mockReset(); + } + }
🧹 Nitpick comments (1)
packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
18-20: Consider documenting the hashing approach.This
hashEmailfunction hashes the entire email (including domain) for PII protection in cache keys, which differs from the existinghashEmailinpackages/lib/server/PiiHasher.ts(which hashes only the local part). While these serve different purposes, adding a brief comment explaining why full-email hashing is used here would prevent confusion.Example addition:
/** * Hashes an email address using SHA-256 to protect PII in cache keys. * This ensures no plaintext email addresses are stored in Redis for privacy/HIPAA compliance. + * Note: Unlike PiiHasher.hashEmail, this hashes the full email (including domain) + * since we need a consistent cache key for the exact email address. */
📜 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 (21)
apps/web/test/utils/bookingScenario/bookingScenario.ts(17 hunks)packages/features/bookings/lib/handleNewBooking.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts(1 hunks)packages/features/di/bookings/tokens.ts(1 hunks)packages/features/di/tokens.ts(1 hunks)packages/features/emailValidation/di/EmailValidation.container.ts(1 hunks)packages/features/emailValidation/di/EmailValidationProviderService.module.ts(1 hunks)packages/features/emailValidation/di/EmailValidationService.module.ts(1 hunks)packages/features/emailValidation/lib/dto/types.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.test.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts(1 hunks)packages/features/emailValidation/lib/validateBookingEmail.ts(1 hunks)packages/features/flags/config.ts(1 hunks)packages/features/flags/hooks/index.ts(1 hunks)packages/features/redis/di/redisModule.ts(1 hunks)packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql(1 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/features/di/tokens.ts
- packages/features/emailValidation/di/EmailValidationProviderService.module.ts
- packages/features/di/bookings/tokens.ts
- packages/features/flags/config.ts
- packages/features/emailValidation/lib/validateBookingEmail.ts
- packages/features/redis/di/redisModule.ts
- packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/emailValidation/di/EmailValidation.container.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/dto/types.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tspackages/features/flags/hooks/index.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/features/emailValidation/lib/service/EmailValidationService.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/emailValidation/di/EmailValidation.container.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/dto/types.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tspackages/features/flags/hooks/index.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/features/emailValidation/lib/service/EmailValidationService.test.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/emailValidation/di/EmailValidation.container.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.tspackages/features/emailValidation/lib/dto/types.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tspackages/features/flags/hooks/index.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.tsapps/web/test/utils/bookingScenario/bookingScenario.tspackages/features/emailValidation/lib/service/EmailValidationService.test.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/emailValidation/lib/service/EmailValidationService.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
🧬 Code graph analysis (9)
packages/features/emailValidation/di/EmailValidation.container.ts (3)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
IEmailValidationService(12-36)packages/features/emailValidation/di/EmailValidationService.module.ts (1)
emailValidationServiceModule(9-9)packages/features/di/tokens.ts (1)
DI_TOKENS(3-59)
packages/features/emailValidation/di/EmailValidationService.module.ts (4)
packages/features/di/di.ts (2)
bindModuleToClassOnToken(57-113)ModuleLoader(4-4)packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
EmailValidationService(44-284)packages/features/emailValidation/di/EmailValidationProviderService.module.ts (2)
emailValidationProviderServiceLoader(11-17)moduleLoader(19-22)packages/features/redis/di/redisModule.ts (1)
moduleLoader(19-25)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/features/emailValidation/lib/validateBookingEmail.ts (1)
validateBookingEmail(34-127)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts (1)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
ZeroBounceEmailValidationProviderService(17-120)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
packages/features/emailValidation/lib/dto/types.ts (1)
EmailValidationRequest(3-6)
packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts (5)
packages/features/redis/RedisService.ts (1)
RedisService(5-50)packages/features/redis/NoopRedisService.ts (1)
NoopRedisService(7-32)apps/web/test/utils/bookingScenario/bookingScenario.ts (8)
enableEmailValidationForTeam(1690-1722)getBooker(2270-2284)getOrganizer(1530-1589)TestData(1249-1521)getGoogleCalendarCredential(1202-1210)createBookingScenario(987-1018)getScenarioData(1591-1677)mockCalendarToHaveNoBusySlots(2015-2028)packages/features/bookings/lib/handleNewBooking/test/getNewBookingHandler.ts (1)
getNewBookingHandler(6-8)apps/web/test/utils/bookingScenario/expects.ts (2)
expectBookingToBeInDatabase(418-435)expectSuccessfulBookingCreationEmails(459-645)
packages/features/emailValidation/lib/service/EmailValidationService.ts (5)
packages/lib/server/PiiHasher.ts (1)
hashEmail(18-22)packages/features/emailValidation/lib/dto/types.ts (4)
validationStatusSchema(13-50)EmailValidationStatus(52-52)EmailValidationResult(54-65)EmailValidationRequest(3-6)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (2)
IEmailValidationService(12-36)EmailValidationResponse(3-5)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
packages/features/emailValidation/lib/dto/types.ts (2)
EmailValidationRequest(3-6)EmailValidationResult(54-65)
packages/features/emailValidation/lib/service/EmailValidationService.test.ts (4)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
EmailValidationService(44-284)packages/features/emailValidation/lib/dto/types.ts (2)
EmailValidationRequest(3-6)EmailValidationResult(54-65)
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (30)
apps/web/test/utils/bookingScenario/bookingScenario.ts (3)
1689-1722: Previous concern addressed - LGTM!The
upsertoperation now correctly enables the feature in both theupdateandcreatebranches (line 1694-1696). This ensures the test helper always enables thebooking-email-validationfeature regardless of its prior state.
1823-1823: Good addition for testing async scenarios.The
getAvailabilitySlowDownTimeparameter allows tests to simulate slow availability checks, which is useful for validating timeout handling and parallel execution behavior.Also applies to: 1981-1983
2416-2428: Improved flexibility for test scenarios.Using
emailFielddirectly when provided allows tests to customize the email field configuration while maintaining a sensible default fallback.packages/features/flags/hooks/index.ts (1)
31-31: LGTM!The new feature flag is correctly added with the default value set to
false, aligning with the PR's objective of having the feature disabled by default and enabled per team/org via migration.packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts (4)
28-99: LGTM! Well-structured test setup.The test setup is well-organized with proper mock initialization, environment variable management, and feature flag enablement. The SHA-256 hashing for cache keys aligns with the privacy requirements mentioned in the PR objectives.
101-182: LGTM! Comprehensive valid email test.The test properly validates the happy path where email validation succeeds, booking is created, and confirmation emails are sent. Good coverage of the success scenario.
184-251: LGTM! Invalid email rejection test is thorough.The test correctly validates that bookings with invalid emails are rejected with the appropriate error message. The delay ensures the validation promise has time to resolve before assertions.
344-408: LGTM! Cached validation test is well-structured.The test properly validates the cache-hit scenario where a previously validated email result is retrieved from Redis, avoiding unnecessary provider calls. Good verification of the cache key format.
packages/features/bookings/lib/handleNewBooking.ts (4)
27-29: LGTM! DI container integration.The imports correctly use the new DI container getters for services, aligning with the architectural pattern introduced in this PR for dependency injection.
91-91: LGTM! Email validation utility import.The import correctly brings in the
validateBookingEmailfunction from the new email validation module.
500-508: LGTM! Parallel email validation implementation.The email validation is correctly integrated with a two-phase approach:
- Fast checks (cache + Cal.com DB) execute immediately
- Provider validation starts in parallel without blocking
This aligns with the PR's objective to mask provider latency by running validation concurrently with availability checks.
1408-1409: LGTM! Provider validation await placement.The validation await is correctly placed before booking creation for the main flow. However, this doesn't protect early return paths (see previous comment).
packages/features/emailValidation/lib/dto/types.ts (3)
1-6: LGTM! Clean request interface.The
EmailValidationRequestinterface is well-defined with required email and optional IP address for context.
8-50: LGTM! Comprehensive status schema with excellent documentation.The validation status schema is well-designed with:
- Clear, canonical statuses that all providers must map to
- Detailed documentation explaining each status
- Custom Cal.com-specific statuses for verified emails and provider fallback
- Type safety through Zod
52-65: LGTM! Well-documented result interface.The
EmailValidationResultinterface clearly separates the canonical status (used for blocking decisions) from optional provider-specific sub-status (used only for logging/debugging). Good design choice.packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql (1)
1-9: LGTM! Safe and idempotent migration.The migration correctly:
- Inserts the feature flag with appropriate metadata
- Uses
ON CONFLICT (slug) DO NOTHINGfor idempotency- Sets default
enabledtofalse, matching the PR's rollout strategy- Uses
OPERATIONALtype, appropriate for a runtime behavior flagturbo.json (1)
289-290: LGTM! Environment variable registration.The
ZEROBOUNCE_API_KEYis correctly added toglobalEnv, making it available to the ZeroBounce email validation provider service across the monorepo.packages/features/emailValidation/di/EmailValidation.container.ts (1)
1-12: LGTM! DI container implementation.The container setup follows the established DI pattern:
- Creates a singleton container at module level
- Loads the email validation service module
- Resolves and returns the service instance via the
EMAIL_VALIDATION_SERVICEtokenThis pattern is consistent with other DI containers in the codebase.
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts (6)
1-23: LGTM! Well-structured test setup.The test setup properly:
- Mocks the global
fetchfunction- Manages environment variables (API key) with proper cleanup
- Provides test isolation through
beforeEachandafterEachhooks
26-48: LGTM! Valid status test.The test correctly validates that the service returns the status as provided by ZeroBounce API, with both status and subStatus properly mapped.
50-60: LGTM! API key validation test.The test properly validates that the service rejects validation requests when the API key is not configured, and confirms that no API call is made in this case.
62-68: LGTM! Network error propagation test.The test correctly validates that network errors are propagated to the caller, allowing higher-level error handling.
70-82: LGTM! Server error handling test.The test properly validates that non-OK HTTP responses from ZeroBounce API are converted to descriptive error messages.
84-108: LGTM! IP address context test.The test correctly validates that when an IP address is provided, it's properly included in the ZeroBounce API request as a query parameter.
packages/features/emailValidation/di/EmailValidationService.module.ts (1)
1-29: LGTM!The DI module correctly wires EmailValidationService with its three dependencies (emailValidationProvider, userRepository, redisService). The structure follows the established pattern and aligns with the service's constructor signature.
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
1-19: LGTM!The interface is well-designed with clear separation of concerns. Provider services handle validation and mapping to canonical statuses, while blocking decisions remain centralized in EmailValidationService. The AbortSignal support enables proper timeout handling.
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
1-36: LGTM!The interface clearly separates the fast path (validateWithCalcom with cache) from the slow path (validateWithProvider). The intentional restriction of
skipCache: trueis well-documented, and the two response types appropriately distinguish between paths that return cache continuation flags vs. simple blocking decisions.packages/features/emailValidation/lib/service/EmailValidationService.test.ts (1)
1-357: Excellent test coverage!The test suite comprehensively covers:
- Cache-first flows (hits, misses, case-insensitivity)
- Cal.com verification paths (verified, unverified, non-existent users)
- Provider validation paths (success, blocking statuses, timeouts, failures)
- Error handling and fallback scenarios
- Non-hammering behavior when cache or DB is down
The tests verify correct TTLs, blocking decisions, and proper cache key hashing. Well-structured with clear test names and good use of mocks.
packages/features/emailValidation/lib/service/EmailValidationService.ts (2)
24-25: Verify the cache TTL against documented requirements.The current implementation uses a 24-hour TTL, which aligns with the PR summary ("24-hour TTL for normal results"). However, a past review comment suggested this should be 2 hours. Please confirm whether 24 hours is the correct TTL or if it should be reduced to 2 hours.
67-283: Strong implementation with excellent resilience patterns.The service demonstrates solid engineering practices:
- Cache-first architecture: Redis → Cal.com DB → Provider flow minimizes latency and cost
- Graceful degradation: Multiple fallback layers ensure bookings proceed when Redis, provider, or DB fail
- Non-hammering protection:
continueWithProviderflag prevents cascade failures when cache is down (lines 143-148)- Timeout handling: AbortController + Promise.race pattern properly enforces 3s provider timeout with cleanup (lines 227-241)
- PII protection: SHA-256 hashing of emails in cache keys (line 18)
- Conditional TTLs: 5-minute cache for provider failures prevents repeated failed calls (lines 74-77)
- Error recovery: All cache/DB operations are non-fatal with appropriate logging (lines 83-91, 106-113)
The implementation correctly balances validation strictness with availability requirements.
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
from
fe43010 to
0d2214e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
68-75: Mask PII before logging ZeroBounce errorsThe error log is emitting the full email address and raw provider payload (which echoes the email). That violates the privacy requirement called out in the PR objectives (truncate emails to the first 10 chars) and risks leaking PII into logs. Please redact the email and sanitize the response before logging.
- this.logger.error( - "ZeroBounce API returned invalid response format", - safeStringify({ - email: email, - rawResponse: rawData, - validationErrors: validationResult.error.issues, - }) - ); + const maskedEmail = `${email.slice(0, 10)}***`; + const redactedResponse = { + ...rawData, + address: + typeof rawData.address === "string" ? `${rawData.address.slice(0, 10)}***` : rawData.address, + account: + typeof (rawData as Record<string, unknown>).account === "string" + ? `${String((rawData as Record<string, unknown>).account).slice(0, 3)}***` + : (rawData as Record<string, unknown>).account, + }; + + this.logger.error( + "ZeroBounce API returned invalid response format", + safeStringify({ + email: maskedEmail, + rawResponse: redactedResponse, + validationErrors: validationResult.error.issues, + }) + );As per PR objectives.
packages/features/bookings/lib/handleNewBooking.ts (1)
580-624: Critical: Early return bypasses email validation.The early return at line 615 exits before reaching the validation await at line 1409. If ZeroBounce flags the email as
invalid/spamtrap/abuseduring the parallel provider check, this return path bypasses that result entirely, allowing blocked emails through.The past review comment indicated this was "Fixed" but the issue persists. Every return path between line 508 (where validation starts) and line 1409 (where it's awaited) must await validation first:
if (existingBooking) { + // Wait for email validation to complete before returning + await emailValidationResult?.waitForProviderValidation(); + const hasPayments = existingBooking.payment.length > 0; const isPaidBooking = existingBooking.paid || !hasPayments; // ... rest of early return logicApply this pattern to all early returns between lines 508 and 1409.
packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts (1)
289-342: Restore the fetch mock after this test.This test overrides the global
fetchmock at line 291 but never restores the original ZeroBounce stub. Thevi.clearAllMocks()inbeforeEachclears call history but may not resetmockImplementation, so subsequent tests could inherit the "network error" behavior, causing test-order dependencies.Wrap the override in a try/finally block or use
mockImplementationOnce:- (globalThis.fetch as any).mockImplementation((url: string | URL) => { + const fetchMock = vi.mocked(globalThis.fetch); + const originalImpl = fetchMock.getMockImplementation(); + + fetchMock.mockImplementation((url: string | URL) => { const urlString = url.toString(); if (urlString.includes("zerobounce.net/v2/validate")) { return Promise.reject(new Error("Network error: Failed to connect to ZeroBounce API")); } return Promise.resolve({ ok: false, status: 404, } as Response); }); - const createdBooking = await handleNewBooking({ + try { + const createdBooking = await handleNewBooking({ + bookingData: mockBookingData, + }); - expect(createdBooking.responses).toMatchObject({ + expect(createdBooking.responses).toMatchObject({ + name: booker.name, + email: booker.email, + }); + // ... rest of assertions + } finally { + if (originalImpl) { + fetchMock.mockImplementation(originalImpl); + } else { + fetchMock.mockReset(); + } + }
🧹 Nitpick comments (2)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
2165-2173: Unused parameters in mockPaymentApp.The function accepts
metadataLookupKeyandappStoreLookupKeyparameters but doesn't use them. Consider removing these parameters if they're not needed, or implement the intended logic.Apply this diff to remove the unused parameters:
-// eslint-disable-next-line @typescript-eslint/no-unused-vars -export function mockPaymentApp(data: { metadataLookupKey: string; appStoreLookupKey?: string }) { +export function mockPaymentApp() { const { paymentUid, externalId } = getMockPaymentService(); return {packages/features/emailValidation/di/EmailValidation.container.ts (1)
9-12: Consider making module loading idempotent.
emailValidationServiceModule.loadModule(container)runs on every call togetEmailValidationService(). If this function is invoked multiple times (e.g., across different request handlers), the module gets reloaded repeatedly, which may be inefficient. Consider either:
- Guarding the load with a flag:
if (!loaded) { emailValidationServiceModule.loadModule(container); loaded = true; }- Making
loadModuleitself idempotent so repeated calls are no-ops
📜 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 (21)
apps/web/test/utils/bookingScenario/bookingScenario.ts(17 hunks)packages/features/bookings/lib/handleNewBooking.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts(1 hunks)packages/features/di/bookings/tokens.ts(1 hunks)packages/features/di/tokens.ts(1 hunks)packages/features/emailValidation/di/EmailValidation.container.ts(1 hunks)packages/features/emailValidation/di/EmailValidationProviderService.module.ts(1 hunks)packages/features/emailValidation/di/EmailValidationService.module.ts(1 hunks)packages/features/emailValidation/lib/dto/types.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.test.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts(1 hunks)packages/features/emailValidation/lib/validateBookingEmail.ts(1 hunks)packages/features/flags/config.ts(1 hunks)packages/features/flags/hooks/index.ts(1 hunks)packages/features/redis/di/redisModule.ts(1 hunks)packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql(1 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/features/flags/config.ts
- packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts
- packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts
- turbo.json
- packages/features/di/tokens.ts
- packages/features/emailValidation/lib/dto/types.ts
- packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts
- packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql
- packages/features/emailValidation/di/EmailValidationService.module.ts
- packages/features/emailValidation/di/EmailValidationProviderService.module.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/redis/di/redisModule.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/di/EmailValidation.container.tspackages/features/di/bookings/tokens.tspackages/features/flags/hooks/index.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.tspackages/features/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/validateBookingEmail.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tsapps/web/test/utils/bookingScenario/bookingScenario.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/redis/di/redisModule.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/di/EmailValidation.container.tspackages/features/di/bookings/tokens.tspackages/features/flags/hooks/index.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.tspackages/features/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/validateBookingEmail.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tsapps/web/test/utils/bookingScenario/bookingScenario.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/redis/di/redisModule.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/di/EmailValidation.container.tspackages/features/di/bookings/tokens.tspackages/features/flags/hooks/index.tspackages/features/bookings/lib/handleNewBooking/test/email-validation.test.tspackages/features/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/validateBookingEmail.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.tsapps/web/test/utils/bookingScenario/bookingScenario.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/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts
🧠 Learnings (2)
📓 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.
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
🧬 Code graph analysis (8)
packages/features/redis/di/redisModule.ts (4)
packages/features/di/di.ts (2)
ModuleLoader(4-4)Container(114-114)packages/features/di/tokens.ts (1)
DI_TOKENS(3-59)packages/features/di/modules/Cache.ts (1)
moduleLoader(14-19)packages/prisma/prisma.module.ts (1)
moduleLoader(13-19)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/features/emailValidation/lib/validateBookingEmail.ts (1)
validateBookingEmail(34-127)
packages/features/emailValidation/di/EmailValidation.container.ts (3)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
IEmailValidationService(12-36)packages/features/emailValidation/di/EmailValidationService.module.ts (1)
emailValidationServiceModule(9-9)packages/features/di/tokens.ts (1)
DI_TOKENS(3-59)
packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts (5)
packages/features/redis/RedisService.ts (1)
RedisService(5-50)packages/features/redis/NoopRedisService.ts (1)
NoopRedisService(7-32)apps/web/test/utils/bookingScenario/bookingScenario.ts (8)
enableEmailValidationForTeam(1690-1722)getBooker(2270-2284)getOrganizer(1530-1589)TestData(1249-1521)getGoogleCalendarCredential(1202-1210)createBookingScenario(987-1018)getScenarioData(1591-1677)mockCalendarToHaveNoBusySlots(2015-2028)packages/features/bookings/lib/handleNewBooking/test/getNewBookingHandler.ts (1)
getNewBookingHandler(6-8)apps/web/test/utils/bookingScenario/expects.ts (2)
expectBookingToBeInDatabase(418-435)expectSuccessfulBookingCreationEmails(459-645)
packages/features/emailValidation/lib/service/EmailValidationService.test.ts (4)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
EmailValidationService(44-284)packages/features/emailValidation/lib/dto/types.ts (2)
EmailValidationRequest(3-6)EmailValidationResult(54-65)
packages/features/emailValidation/lib/service/EmailValidationService.ts (5)
packages/lib/server/PiiHasher.ts (1)
hashEmail(18-22)packages/features/emailValidation/lib/dto/types.ts (4)
validationStatusSchema(13-50)EmailValidationStatus(52-52)EmailValidationResult(54-65)EmailValidationRequest(3-6)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (2)
IEmailValidationService(12-36)EmailValidationResponse(3-5)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)
packages/features/emailValidation/lib/validateBookingEmail.ts (3)
packages/features/emailValidation/lib/dto/types.ts (1)
EmailValidationResult(54-65)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
EmailValidationResponse(3-5)packages/features/emailValidation/di/EmailValidation.container.ts (1)
getEmailValidationService(9-12)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (2)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/emailValidation/lib/dto/types.ts (3)
EmailValidationRequest(3-6)EmailValidationResult(54-65)EmailValidationStatus(52-52)
🔇 Additional comments (11)
apps/web/test/utils/bookingScenario/bookingScenario.ts (3)
1689-1722: Past review comment addressed.The
updatebranch now correctly setsenabled: trueandtype: "OPERATIONAL"to ensure the feature is always enabled for tests, addressing the concern from the previous review.
1823-1823: LGTM! Useful test helper for simulating slow availability checks.The
getAvailabilitySlowDownTimeparameter allows tests to simulate latency in availability checks, which is valuable for testing parallel execution flows mentioned in the PR objectives.Also applies to: 1979-1983
2416-2428: LGTM! Flexible email field handling.The conditional logic properly uses the provided
emailFieldif present, otherwise falls back to the default email field configuration. This allows tests to customize email field behavior when needed.packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts (1)
28-99: LGTM! Test setup is well-structured.The test setup properly mocks Redis services, ZeroBounce API, and configures the feature flag for email validation. The
beforeEachhook ensures clean state between tests.packages/features/bookings/lib/handleNewBooking.ts (1)
500-508: LGTM! Email validation setup follows the documented two-phase pattern.The fast Cal.com validation runs immediately, and the slow provider validation starts in parallel without blocking. This design properly masks provider latency by running it alongside availability checks.
packages/features/emailValidation/lib/service/EmailValidationService.test.ts (2)
22-29: LGTM! Cache key helper maintains PII protection.The
getExpectedCacheKeyhelper correctly mirrors the hashing implementation (SHA-256 of lowercase, trimmed email) used in the service, ensuring tests verify the actual cache keys without exposing plaintext emails.
49-356: Excellent test coverage for EmailValidationService.The test suite thoroughly exercises:
- Cache-driven flows (hit/miss, case-insensitive keys)
- Cal.com verification flows (verified/unverified users)
- Provider validation flows (valid/invalid, timeouts, errors)
- Proper TTL values (24h for normal, 5min for fallback)
- Error handling (Redis failures, DB failures, cache set failures)
packages/features/emailValidation/lib/service/EmailValidationService.ts (4)
24-28: Clarify cache TTL policy: 2 hours or 24 hours?There's an inconsistency that needs resolution:
- Past review comment requested changing the TTL from 24 hours to 2 hours, stating "the PR summary/architecture promise a 2‑hour cache"
- Current PR objectives state: "Redis caching (24-hour TTL for normal results, 5-minute TTL for provider failures)"
- Current implementation uses 24 hours (line 25)
Either:
- The PR summary was updated after the review to reflect 24h as the intended policy, or
- The implementation should be changed to 2 hours as the past review requested
Please clarify which TTL is correct. If 24 hours is the intended policy, the past review comment should be marked as outdated. If 2 hours is correct, apply the suggested change.
14-22: LGTM! Email hashing properly protects PII in cache keys.The SHA-256 hashing with lowercase normalization and trimming ensures no plaintext emails leak into Redis, addressing privacy/HIPAA requirements.
217-275: LGTM! Provider timeout handling is robust.The
validateWithProviderInternalmethod correctly:
- Enforces 3s timeout using AbortController
- Races provider validation against timeout
- Cleans up timeout in both success and error paths
- Falls back to
calcom-provider-fallbackstatus on failure- Logs duration metrics for observability
117-173: LGTM! The fast path correctly prevents provider hammering when cache is unavailable.
validateWithCalcomproperly:
- Checks cache first (single lookup)
- Falls back to Cal.com DB on cache miss
- Returns
continueWithProvider: falsewhen cache read fails (line 148), preventing cost explosion and rate limiting issues- Caches verified emails from Cal.com DB
The inline comment at lines 143-147 clearly explains the hammering prevention logic.
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
from
0d2214e to
6ea8e87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/features/bookings/lib/handleNewBooking.ts (3)
615-623: Critical: Early return bypasses provider validation.The existingBooking path returns immediately without awaiting
emailValidationResult?.waitForProviderValidation(). If ZeroBounce flags the email during the parallel validation started at line 508, that rejection is trapped but never checked here, allowing blocked/invalid addresses to slip through.Apply this fix before the early return:
if (existingBooking) { + // Ensure provider validation completes before returning + await emailValidationResult?.waitForProviderValidation(); + const hasPayments = existingBooking.payment.length > 0; const isPaidBooking = existingBooking.paid || !hasPayments;This same pattern must be applied to all early return paths between line 508 and line 1409.
1408-1410: Verify all return paths await validation.The validation is correctly awaited here before booking creation. However, ensure all early return paths (e.g., Line 615) also await this validation. See the critical issue comment for details.
Run this script to identify all return statements between the validation start (Line 508) and this await point:
#!/bin/bash # Find all return statements in handleNewBooking between validation start and await sed -n '508,1409p' packages/features/bookings/lib/handleNewBooking.ts | grep -n "return {" | head -20
500-509: Early return at Line 615 bypasses email validation.The provider validation promise is started at Line 508 but only awaited at Line 1409. The early return for
existingBookingat Lines 615-623 exits before the validation completes, allowing blocked/invalid emails to slip through on subsequent booking attempts.Despite the previous review comment being marked as "Fixed", the issue persists: the validation promise is not awaited before this early return.
Apply this fix before the early return:
if (existingBooking) { + // Wait for email validation to complete before returning + await emailValidationResult?.waitForProviderValidation(); + const hasPayments = existingBooking.payment.length > 0; const isPaidBooking = existingBooking.paid || !hasPayments;Alternatively, enforce validation completion before all return paths by wrapping the main logic in a try/finally block that awaits the validation promise in the finally clause.
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
68-76: PII exposure contradicts documented privacy requirements.Lines 71-72 log the full email address and
rawResponse(which echoesaddress), but the PR objectives explicitly state: "PII protection: cache keys use SHA-256 hashed emails; plaintext emails are not logged."While full diagnostics are valuable, this creates a compliance gap. Consider a middle ground:
+ const maskedEmail = email.length > 10 ? `${email.slice(0, 10)}***` : email; + const redactedResponse = { + ...rawData, + address: typeof rawData.address === "string" ? `${String(rawData.address).slice(0, 10)}***` : rawData.address, + }; + this.logger.error( "ZeroBounce API returned invalid response format", safeStringify({ - email: email, - rawResponse: rawData, + email: maskedEmail, + rawResponse: redactedResponse, validationErrors: validationResult.error.issues, }) );This preserves debugging context (validation errors, status codes, sub_status) while redacting email addresses per the documented policy.
🧹 Nitpick comments (1)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (1)
67-76: Consider masking email addresses in error logs.The PR objectives document a PII protection requirement: "emails truncated to first 10 chars in logs." Currently, the full email is logged at Line 71, and
rawResponseat Line 72 also contains the full email address field.While keeping full context for investigation is valuable, consider this middle-ground approach:
Apply this diff to mask email addresses while preserving diagnostic information:
+ const maskedEmail = `${email.slice(0, 10)}***`; + const redactedResponse = { + ...rawData, + address: typeof rawData.address === "string" ? `${rawData.address.slice(0, 10)}***` : rawData.address, + }; + this.logger.error( "ZeroBounce API returned invalid response format", safeStringify({ - email: email, - rawResponse: rawData, + email: maskedEmail, + rawResponse: redactedResponse, validationErrors: validationResult.error.issues, }) );This preserves
validationErrorsand other diagnostic fields while masking the email addresses.
📜 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 (21)
apps/web/test/utils/bookingScenario/bookingScenario.ts(2 hunks)packages/features/bookings/lib/handleNewBooking.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts(1 hunks)packages/features/di/bookings/tokens.ts(1 hunks)packages/features/di/tokens.ts(1 hunks)packages/features/emailValidation/di/EmailValidation.container.ts(1 hunks)packages/features/emailValidation/di/EmailValidationProviderService.module.ts(1 hunks)packages/features/emailValidation/di/EmailValidationService.module.ts(1 hunks)packages/features/emailValidation/lib/dto/types.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.test.ts(1 hunks)packages/features/emailValidation/lib/service/EmailValidationService.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts(1 hunks)packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts(1 hunks)packages/features/emailValidation/lib/validateBookingEmail.ts(1 hunks)packages/features/flags/config.ts(1 hunks)packages/features/flags/hooks/index.ts(1 hunks)packages/features/redis/di/redisModule.ts(1 hunks)packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql(1 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/prisma/migrations/20251001130927_add_feature_flag_booking_email_validation/migration.sql
- packages/features/di/tokens.ts
- turbo.json
- packages/features/emailValidation/lib/dto/types.ts
- packages/features/emailValidation/di/EmailValidation.container.ts
- packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts
- packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.test.ts
- packages/features/bookings/lib/handleNewBooking/test/email-validation.test.ts
- packages/features/emailValidation/lib/validateBookingEmail.ts
- apps/web/test/utils/bookingScenario/bookingScenario.ts
- packages/features/flags/config.ts
- packages/features/di/bookings/tokens.ts
- packages/features/redis/di/redisModule.ts
- packages/features/flags/hooks/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/emailValidation/di/EmailValidationProviderService.module.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.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/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/emailValidation/di/EmailValidationProviderService.module.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.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/emailValidation/lib/service/EmailValidationService.test.tspackages/features/emailValidation/di/EmailValidationService.module.tspackages/features/emailValidation/di/EmailValidationProviderService.module.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/emailValidation/lib/service/IEmailValidationService.interface.tspackages/features/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.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/emailValidation/lib/service/EmailValidationService.tspackages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
🧬 Code graph analysis (7)
packages/features/emailValidation/lib/service/EmailValidationService.test.ts (4)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
EmailValidationService(44-284)packages/features/emailValidation/lib/dto/types.ts (2)
EmailValidationRequest(3-6)EmailValidationResult(54-65)
packages/features/emailValidation/di/EmailValidationService.module.ts (5)
packages/features/di/di.ts (2)
bindModuleToClassOnToken(57-113)ModuleLoader(4-4)packages/features/di/tokens.ts (1)
DI_TOKENS(3-59)packages/features/emailValidation/lib/service/EmailValidationService.ts (1)
EmailValidationService(44-284)packages/features/emailValidation/di/EmailValidationProviderService.module.ts (2)
emailValidationProviderServiceLoader(11-17)moduleLoader(19-22)packages/features/redis/di/redisModule.ts (1)
moduleLoader(19-25)
packages/features/emailValidation/di/EmailValidationProviderService.module.ts (2)
packages/features/di/di.ts (2)
bindModuleToClassOnToken(57-113)ModuleLoader(4-4)packages/features/di/tokens.ts (1)
DI_TOKENS(3-59)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/features/emailValidation/lib/validateBookingEmail.ts (1)
validateBookingEmail(34-127)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (1)
packages/features/emailValidation/lib/dto/types.ts (1)
EmailValidationRequest(3-6)
packages/features/emailValidation/lib/service/EmailValidationService.ts (5)
packages/lib/server/PiiHasher.ts (1)
hashEmail(18-22)packages/features/emailValidation/lib/dto/types.ts (4)
validationStatusSchema(13-50)EmailValidationStatus(52-52)EmailValidationResult(54-65)EmailValidationRequest(3-6)packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (2)
IEmailValidationService(12-36)EmailValidationResponse(3-5)packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/redis/IRedisService.d.ts (1)
IRedisService(1-13)
packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (2)
packages/features/emailValidation/lib/service/IEmailValidationProviderService.interface.ts (1)
IEmailValidationProviderService(14-19)packages/features/emailValidation/lib/dto/types.ts (3)
EmailValidationRequest(3-6)EmailValidationResult(54-65)EmailValidationStatus(52-52)
🔇 Additional comments (23)
packages/features/emailValidation/lib/service/IEmailValidationService.interface.ts (2)
1-36: LGTM!The interface definition is clean, well-documented, and provides a clear contract for the email validation service with appropriate type safety.
1-36: LGTM! Clean interface design with clear separation of concerns.The interface cleanly separates the fast Cal.com verification path from the slower provider-based validation, and the documentation clearly explains the two-phase approach. The
skipCache: trueliteral type appropriately enforces that only skip-cache mode is supported invalidateWithProvider.packages/features/emailValidation/di/EmailValidationService.module.ts (2)
1-29: LGTM!The DI module wiring correctly binds the EmailValidationService with its three dependencies (emailValidationProvider, userRepository, redisService) and follows the established module pattern.
1-29: LGTM! DI module is correctly wired.The module properly binds EmailValidationService with its three dependencies (emailValidationProvider, userRepository, redisService) using the standard DI pattern. The loader configuration matches the constructor signature shown in the service implementation.
packages/features/emailValidation/di/EmailValidationProviderService.module.ts (2)
1-22: LGTM!The provider service module correctly wires ZeroBounceEmailValidationProviderService with no dependencies (empty depsMap), which matches the provider's constructor signature.
1-22: LGTM! Provider module is correctly configured.The module correctly wires the ZeroBounce provider service with an empty
depsMap(since the provider has no constructor dependencies) and follows the standard DI module pattern.packages/features/bookings/lib/handleNewBooking.ts (5)
500-508: Email validation flow initiated correctly.The fast path validation (validateWithCalcom) executes synchronously and blocks invalid emails immediately. The slow path (provider validation) is started in parallel to mask latency during availability checks.
1408-1409: Provider validation correctly awaited in main flow.The await ensures provider validation completes before creating new bookings. However, this protection does not cover early return paths (see comment on lines 615-623).
27-29: LGTM! Clean refactor to use DI containers.The imports correctly replace the previous internal service references with DI container getters, aligning with the new DI-based architecture.
91-91: LGTM! Email validation utility imported.The
validateBookingEmailhelper is appropriately imported for the fast/slow validation flow.
1494-1498: LGTM! Formatting adjustment.The conditional logic for
changedOrganizerremains functionally equivalent; this is just a formatting change.packages/features/emailValidation/lib/service/EmailValidationService.test.ts (2)
1-357: LGTM!Comprehensive test suite with excellent coverage of cache flows, Cal.com verification, provider validation (including timeouts and failures), and error handling. The previous
it.onlyissue has been resolved.
1-357: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Cache-driven flows (valid/invalid cached results, case-insensitive keys)
- Cal.com verification flows (verified/unverified users)
- Provider validation flows (valid/invalid statuses, timeouts, errors)
- Error handling (Redis failures, DB failures, cache set failures)
- TTL behavior and fallback scenarios
The previous
it.onlyissue has been resolved, and all tests will now run in CI.packages/features/emailValidation/lib/service/ZeroBounceEmailValidationProviderService.ts (2)
48-56: LGTM!The fetch call now correctly passes the
abortSignalparameter, allowing the timeout mechanism in EmailValidationService to properly cancel long-running requests.
48-56: LGTM! AbortSignal is now properly wired.The
abortSignalis correctly passed to the fetch call, allowing the timeout mechanism in EmailValidationService to properly cancel long-running requests.packages/features/emailValidation/lib/service/EmailValidationService.ts (8)
24-28: Cache TTL correctly implements documented policy.The 24-hour TTL for normal validation results and 5-minute TTL for provider failures match the architecture documented in the PR objectives. The past review comment referencing a "2-hour policy" appears to be based on outdated requirements.
67-92: LGTM!The cache-set error handling is appropriate: failures are logged but don't block the validation flow, ensuring resilience when Redis is unavailable.
122-173: LGTM!The validateWithCalcom flow correctly implements the fast-path validation:
- Cache-first lookup for performance
- Cal.com DB verification for verified users
- Appropriate fallback behavior when cache or DB fails (doesn't hammer the provider)
- The
continueWithProviderflag properly gates provider calls when infrastructure is degraded
220-275: Provider timeout and fallback handling is robust.The 3-second timeout with AbortController, graceful fallback to "calcom-provider-fallback" status, and 5-minute cache TTL for failures effectively prevent provider hammering while maintaining booking availability.
18-28: LGTM! Cache configuration aligns with PR objectives.The cache TTL values correctly implement the documented architecture:
- 24-hour TTL for normal validation results (Line 25)
- 5-minute TTL for provider fallback results (Line 28)
The
hashEmailfunction appropriately uses SHA-256 to protect PII in cache keys.
67-115: LGTM! Robust cache error handling.Both
setInCacheandgetFromCacheproperly handle Redis errors without failing the validation flow. Cache failures are logged but don't block bookings, which is the correct resilience behavior.
122-173: LGTM! Smart fallback logic prevents provider hammering.The
validateWithCalcommethod implements excellent resilience:
- Returns
continueWithProvider: falsewhen cache read fails (Line 148), preventing cascading failures to ZeroBounce- Properly handles Cal.com-verified emails by caching them
- Falls back safely on DB errors
The comment at Lines 143-147 clearly explains the reasoning behind not calling the provider when cache is unavailable.
220-275: LGTM! Provider validation with proper timeout and fallback.The timeout implementation is correct:
- AbortController properly signals cancellation to the provider
- Timeout is enforced via Promise.race
- Fallback result (
calcom-provider-fallback) allows bookings to proceed when provider fails- Timing information is logged for monitoring
hariombalhara
force-pushed
the
10-01-feat_zero-bounce-email-validation
branch
from
6ea8e87 to
f975406
Compare
|
This PR is being marked as stale due to inactivity. |
|
@hariombalhara any update on this? should we ask for foundation review here? |
|
The validation for a new email needs to happen from zerobounce which can potentially delay the booking itself, which we really want to avoid. If we implement such a validation then we want a system where we let the booking happen and cancel it later if it is found to be invalid. Such a flow doesn't slow down bookings. Also, we could potentially use zerobounce in signup to validate emails and avoid spam, which we can decide to pick up later. |
3 participants
Summary
This PR introduces ZeroBounce email validation for team event bookings to prevent spam, abuse, and invalid email addresses from creating bookings on the Cal.com platform.
What This Feature Does
Core Functionality
invalid,spamtrap,abuse, ordo-not-mailFeature Flag Control
booking-email-validationfeature flag (OPERATIONAL type)Architecture
The implementation uses a 2-layer architecture with integrated caching:
Layer 1: EmailValidationService (Service Layer)
The main orchestration service that handles all cross-cutting concerns:
Key Responsibilities:
Two Validation Paths:
validateWithCalcom()- Fast PathshouldBlockandcontinueWithProviderflagsvalidateWithProvider()- Slow PathLayer 2: ZeroBounceEmailValidationProviderService (Provider Layer)
Direct integration with the ZeroBounce validation API:
Key Responsibilities:
https://api.zerobounce.net/v2/validate)ZEROBOUNCE_API_KEYenvironment variableCanonical Status System
The feature defines 9 canonical email validation statuses that all providers must map to:
Standard Statuses (from ZeroBounce)
valid- Confirmed to be valid and safe, very low bounce rateinvalid- Email address is invalid (blocks booking)catch-all- Server accepts all emails, cannot validate without sendingunknown- Cannot validate (offline server, anti-spam systems, etc.)spamtrap- Known spam trap address (blocks booking)abuse- Associated with known spam/abuse behavior (blocks booking)do-not-mail- Valid but should be avoided: disposable, role-based, toxic (blocks booking)Cal.com-Specific Statuses
calcom-verified-email- Email already verified by Cal.com account (skips external validation)calcom-provider-fallback- Provider validation failed, temporarily allowing booking (cached 5 minutes)Blocking Logic
The service blocks bookings for exactly 4 statuses:
invalid,spamtrap,abuse,do-not-mail. All other statuses allow bookings to proceed.Integration & Performance Optimization
Booking Flow Integration
The validation is integrated into
handleNewBooking.tsusing a fast/slow path split:Zero Latency Impact
This architecture is critical for performance because:
Cache Hit Scenario
When the email is cached (cache hit rate should be >80% in production):
Resilience & Fault Tolerance
The implementation includes multiple layers of fault tolerance to ensure bookings never fail due to validation infrastructure issues:
Layer 1: Redis Cache Failures
Behavior: When Redis is unavailable:
shouldBlock: false, continueWithProvider: falseLayer 2: Provider API Failures
Behavior: When ZeroBounce API fails:
status: "calcom-provider-fallback"Layer 3: Provider Timeouts
Behavior: 3-second timeout at service layer:
Layer 4: Cal.com DB Failures
Behavior: When user lookup fails:
Layer 5: Top-Level Error Handling
Behavior: Catches all non-HttpError exceptions:
Result: The system maintains 99.9%+ availability for bookings regardless of external dependencies.
Privacy & Security
PII Protection
Third-Party Data Sharing
API Key Security
ZEROBOUNCE_API_KEYread from environment variablesTesting
Integration Tests (4 Scenarios)
Comprehensive end-to-end tests in
handleNewBooking/test/email-validation.test.ts:Valid Email Acceptance
Invalid Email Rejection
invalidstatus from ZeroBounceAPI Failure Fallback
Cache Hit Behavior
invalidstatusService-Level Unit Tests
Additional tests in
EmailValidationService.test.tscover:Database Changes
Feature Flag Migration
Migration file:
20251001130927_add_feature_flag_booking_email_validation/migration.sqlAdds feature flag to control email validation:
booking-email-validationDeployment Strategy
Prerequisites
ZEROBOUNCE_API_KEYin productionRecommended Rollout
Phase 1 - Pilot (1-2 weeks)
Phase 2 - Expanded Testing (1-2 weeks)
Phase 3 - Gradual Rollout
Monitoring Recommendations
Set up dashboards tracking:
Cost Considerations
Security & Privacy Considerations
Data Privacy
Security Best Practices
Architecture Decisions
Why 2-Layer Architecture?
Why Fast/Slow Path Split?
Why Canonical Status System?
Why Multiple Fallback Layers?
Link to Devin run: https://app.devin.ai/sessions/4536ef337c2d44f3b49bee29853cb5f2
Requested by: @hariombalhara