Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements configurable rate limiting for Zendesk connectors, allowing users to specify a custom calls_per_minute limit per connector instance. The implementation consists of three main components:
Frontend Configuration: A new number input field is added to the Zendesk connector's advanced settings in web/src/lib/connectors/connectors.tsx. This allows users to specify an API calls per minute limit with helpful documentation referencing Zendesk's official rate limit documentation. The field is optional, maintaining backward compatibility.
Backend Implementation: The ZendeskClient class in backend/onyx/connectors/zendesk/connector.py is restructured to accept a calls_per_minute parameter. The change introduces a new request_with_rate_limit function that dynamically applies rate limiting using the existing rate_limit_builder decorator. This creates a dual-layer approach where proactive client-side rate limiting works alongside the existing reactive 429 response handling.
Testing: A comprehensive unit test in backend/tests/unit/onyx/connectors/zendesk/test_zendesk_rate_limit.py validates the rate limiting functionality using fake time modules to avoid actual delays while testing the timing logic.
The implementation follows established patterns in the codebase, reusing the existing rate limiting infrastructure rather than creating Zendesk-specific logic. It maintains backward compatibility by making the rate limit parameter optional and preserves the existing 429 status code handling as a fallback mechanism.
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it adds optional configuration without breaking existing functionality
- Score reflects well-structured implementation following established patterns, comprehensive testing, and proper backward compatibility
- Pay close attention to the backend connector implementation to ensure rate limiting logic integrates correctly with existing retry mechanisms
3 files reviewed, no comments
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="web/src/lib/connectors/connectors.tsx">
<violation number="1" location="web/src/lib/connectors/connectors.tsx:1076">
Number field will be validated as string; add number-type handling in createConnectorValidationSchema to enforce numeric input (e.g., Yup.number()).</violation>
</file>
<file name="backend/tests/unit/onyx/connectors/zendesk/test_zendesk_rate_limit.py">
<violation number="1" location="backend/tests/unit/onyx/connectors/zendesk/test_zendesk_rate_limit.py:65">
Replacing the whole requests module with a SimpleNamespace is brittle; patch only requests.get on the module instead.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| advanced_values: [], | ||
| advanced_values: [ | ||
| { | ||
| type: "number", |
There was a problem hiding this comment.
Number field will be validated as string; add number-type handling in createConnectorValidationSchema to enforce numeric input (e.g., Yup.number()).
Prompt for AI agents
Address the following comment on web/src/lib/connectors/connectors.tsx at line 1076:
<comment>Number field will be validated as string; add number-type handling in createConnectorValidationSchema to enforce numeric input (e.g., Yup.number()).</comment>
<file context>
@@ -1071,7 +1071,16 @@ For example, specifying .*-support.* as a "channel" will cause the connector to
- advanced_values: [],
+ advanced_values: [
+ {
+ type: "number",
+ label: "API Calls per Minute",
+ name: "calls_per_minute",
</file context>
| # minimal Zendesk list response (articles path) | ||
| return _FakeResponse({"articles": [], "meta": {"has_more": False}}) | ||
|
|
||
| monkeypatch.setattr( |
There was a problem hiding this comment.
Replacing the whole requests module with a SimpleNamespace is brittle; patch only requests.get on the module instead.
Prompt for AI agents
Address the following comment on backend/tests/unit/onyx/connectors/zendesk/test_zendesk_rate_limit.py at line 65:
<comment>Replacing the whole requests module with a SimpleNamespace is brittle; patch only requests.get on the module instead.</comment>
<file context>
@@ -0,0 +1,83 @@
+ # minimal Zendesk list response (articles path)
+ return _FakeResponse({"articles": [], "meta": {"has_more": False}})
+
+ monkeypatch.setattr(
+ zendesk_mod, "requests", types.SimpleNamespace(get=_fake_get), raising=True
+ )
</file context>
2 participants
Description
Addresses https://linear.app/danswer/issue/DAN-1618/address-rate-limiting-for-zendesk
Allow users to specify a requests per minute limit per zendesk connector.
How Has This Been Tested?
tested in UI + unit test
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.
Summary by cubic
Adds per-connector Zendesk API rate limiting so users can cap requests per minute and avoid hitting Zendesk limits. Includes a UI control and backend enforcement with retry/backoff.