-
Notifications
You must be signed in to change notification settings - Fork 9.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add rate limiting and more error handling to Cal.ai #11898
Conversation
feat: add rate limiter
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@DexterStorey is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes! |
@@ -1,4 +1,4 @@ | |||
import { TRPCError } from "@calcom/trpc"; | |||
import { TRPCError } from "@calcom/trpc/server"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!!!IMPORTANT!!!
We needed to swap this TRPC import from client side to server side because the AI app uses it server side.
Rate limiting in our use case Does not work without this.
We need a senior engineer on Cal team to verify that this change is okay.
We don't know if this has upstream repercussions or if it's no prob.
Please don't merge this without checking this out. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks safe to me <3
This is only ever ran on the server in all of use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this looks like an ideal change actually
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - always called on server side so that change is fine
NIT: we should move this to a HTTP error in future as now its not always called within trpc
Fixes: #11916 |
What does this PR do?
This PR adds:
Fixes #11916
Requirement/Documentation
Type of change
How should this be tested?
To test rate limit: send more than 20 emails / day
To test agent loop 500 catch: overspend OpenAI tokens or use a bad OpenAI API key
Mandatory Tasks
Checklist
I haven't read the contributing guideMy code doesn't follow the style guidelines of this projectI haven't commented my code, particularly in hard-to-understand areasI haven't checked if my PR needs changes to the documentationI haven't checked if my changes generate no new warnings