Skip to content

Conversation

Udit-takkar
Copy link
Contributor

@Udit-takkar Udit-takkar commented Oct 13, 2025

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • N/A I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

@graphite-app graphite-app bot requested a review from a team October 13, 2025 11:27
@keithwillcode keithwillcode added core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO labels Oct 13, 2025
Copy link
Contributor

coderabbitai bot commented Oct 13, 2025

Walkthrough

  • Updated packages/features/bot-detection/BotDetectionService.ts to add zod-based runtime validation for eventTypeId via safeParse.
  • If eventTypeId is present and not a valid number, the code now throws an HttpError with status 400 and message "Invalid eventTypeId".
  • The validated numeric value (from parse result) is used when calling eventTypeRepository.getTeamIdByEventTypeId, replacing direct use of the original input.
  • Introduced a new import for zod.
  • No alterations to exported or public entity declarations.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive The PR description is largely boilerplate with placeholder issue numbers and lacks any details about the actual changes, such as the addition of Zod-based eventTypeId validation or related HTTP error handling, making it too vague to inform reviewers. Please update the description to include a concise summary of the runtime validation logic added for eventTypeId, the specific error responses introduced, and the relevant issue references to provide clear context for reviewers.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely summarizes the key change by indicating a fix that adds validation for event types, directly reflecting the code modifications which introduce runtime checks on eventTypeId.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/event-type-validation

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 44a3a9e and 6b93364.

📒 Files selected for processing (1)
  • packages/features/bot-detection/BotDetectionService.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Service files must include Service suffix, use PascalCase matching exported class, and avoid generic names (e.g., MembershipService.ts)

Files:

  • packages/features/bot-detection/BotDetectionService.ts
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/features/bot-detection/BotDetectionService.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/bot-detection/BotDetectionService.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/bot-detection/BotDetectionService.ts
🔇 Additional comments (3)
packages/features/bot-detection/BotDetectionService.ts (3)

3-3: Import is fine but tied to validation placement.

The zod import supports the new runtime validation. However, if validation is moved to the API/controller layer (see comment on lines 37-40), this import may become unnecessary in the service.


44-44: Correct usage of validated value.

Using parseEventTypeId.data ensures the repository receives the validated numeric value. If the runtime validation is retained, this is the correct approach.


37-40: Consider moving validation to the API/controller layer.

The BotDetectionConfig interface already declares eventTypeId?: number (line 11), so TypeScript ensures it's a number if provided. Runtime validation in a service class suggests either the type system is bypassed somewhere in the call chain, or validation is happening at the wrong layer.

Services typically receive pre-validated data from API endpoints. If runtime validation is truly needed, consider:

  1. Moving this validation to the API/controller layer where input validation belongs
  2. Verifying whether the type system is bypassed in the call chain (e.g., via any, type assertions, or untyped external data)

Run the following script to identify where checkBotDetection is called and verify if the type system is bypassed:

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot dosubot bot added the 🐛 bug Something isn't working label Oct 13, 2025

// If no eventTypeId provided, skip bot detection
if (!eventTypeId) {
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eventTypeId can be undefined in dynamic bookings so we would need to change the logic here and use eventSlug as well

Copy link

vercel bot commented Oct 13, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
cal Ignored Ignored Oct 13, 2025 11:38am
cal-eu Ignored Ignored Oct 13, 2025 11:38am

Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really prefer to depend on zod for such a simple check, plus also we should handle this error better, should not throw a http error here

@github-actions github-actions bot marked this pull request as draft October 13, 2025 12:08
Copy link
Contributor

E2E results are ready!

@Udit-takkar
Copy link
Contributor Author

Udit-takkar commented Oct 13, 2025

, plus also we should handle this error better, should not throw a http error here

Why not? this is BAD_REQUEST error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants