-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix: Search organization is getting rate limited #4955
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
Fix: Search organization is getting rate limited #4955
Conversation
|
No significant changes currently retry |
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaced the GraphQL query constant used to fetch organizations from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (17)📓 Common learnings📚 Learning: 2025-10-22T22:00:53.943ZApplied to files:
📚 Learning: 2025-04-09T05:00:49.395ZApplied to files:
📚 Learning: 2025-08-20T18:04:34.194ZApplied to files:
📚 Learning: 2025-08-20T18:04:52.324ZApplied to files:
📚 Learning: 2025-08-20T18:04:23.118ZApplied to files:
📚 Learning: 2024-11-01T12:54:20.857ZApplied to files:
📚 Learning: 2025-11-21T12:42:24.884ZApplied to files:
📚 Learning: 2025-01-15T05:39:30.480ZApplied to files:
📚 Learning: 2024-10-25T19:31:37.405ZApplied to files:
📚 Learning: 2025-11-08T18:14:54.607ZApplied to files:
📚 Learning: 2024-06-30T21:17:16.481ZApplied to files:
📚 Learning: 2025-03-25T14:28:21.436ZApplied to files:
📚 Learning: 2025-08-08T08:14:51.613ZApplied to files:
📚 Learning: 2025-04-18T23:57:31.855ZApplied to files:
📚 Learning: 2025-04-18T23:57:31.855ZApplied to files:
📚 Learning: 2025-02-07T13:00:35.500ZApplied to files:
🧬 Code graph analysis (1)src/screens/OrgList/OrgListMocks.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4955 +/- ##
========================================
Coverage 95.36% 95.36%
========================================
Files 387 387
Lines 15057 15057
Branches 4532 4532
========================================
Hits 14359 14359
Misses 670 670
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR addresses rate limiting issues in the organization search functionality by replacing the heavy ORGANIZATION_LIST query with a lightweight ORGANIZATION_FILTER_LIST query. The key optimization is eliminating the expensive members(first: 32) field fetch and replacing it with a simple isMember boolean flag.
- Replaced
ORGANIZATION_LISTquery withORGANIZATION_FILTER_LISTin OrgList component - Updated all test mocks to use the new lightweight query
- Added test coverage for null data response during organization creation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/screens/OrgList/OrgList.tsx | Updated imports and useQuery hook to use ORGANIZATION_FILTER_LIST instead of ORGANIZATION_LIST, reducing query complexity to prevent rate limiting |
| src/screens/OrgList/OrgList.spec.tsx | Updated all test mocks to use ORGANIZATION_FILTER_LIST query and added new test case for null data response during organization creation |
| docs/docs/auto-docs/screens/OrgList/OrgList/functions/default.md | Auto-generated documentation update reflecting the line number change in source file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai full review and reevaluate all resolved items. If the PR requires any changes, even trivial ones, then give your review a “requested changes” status. If no changes are required then give the PR an “approved” status. |
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.
Changes required: Test mocks in OrgListMocks.ts need to be updated from ORGANIZATION_LIST to ORGANIZATION_FILTER_LIST to match the component changes. See detailed review comment for specific changes needed.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9374 🏁 Script executed: Length of output: 1643 🏁 Script executed: Length of output: 1720 🏁 Script executed: Length of output: 24878 🏁 Script executed: Length of output: 18412 🏁 Script executed: Length of output: 62323 🏁 Script executed: Length of output: 7306 🏁 Script executed: Length of output: 1178 🏁 Script executed: Length of output: 78 @palisadoes, I've completed a comprehensive review of PR #4955. I found a critical issue that will cause test failures. Critical Issue: Test Mocks Not UpdatedThe component file Required ChangesFile:
// Change from:
import {
CURRENT_USER,
ORGANIZATION_LIST,
USER_ORGANIZATION_LIST,
ALL_ORGANIZATIONS_PG,
} from 'GraphQl/Queries/Queries';
// To:
import {
CURRENT_USER,
ORGANIZATION_FILTER_LIST,
USER_ORGANIZATION_LIST,
ALL_ORGANIZATIONS_PG,
} from 'GraphQl/Queries/Queries';
// Change from:
{
request: {
query: ORGANIZATION_LIST,
variables: { filter: '' },
},
result: {
data: {
organizations: organizations,
},
},
},
// To:
{
request: {
query: ORGANIZATION_FILTER_LIST,
variables: { filter: '' },
},
result: {
data: {
organizations: organizations,
},
},
},
// Change from:
{
request: {
query: ORGANIZATION_LIST,
variables: { filter: '' },
},
result: {
data: {
organizations: [],
},
},
},
// To:
{
request: {
query: ORGANIZATION_FILTER_LIST,
variables: { filter: '' },
},
result: {
data: {
organizations: [],
},
},
},
// Change both occurrences from ORGANIZATION_LIST to ORGANIZATION_FILTER_LIST:
{
request: {
query: ORGANIZATION_FILTER_LIST,
variables: { filter: '' },
},
result: {
data: {
organizations: organizations,
},
},
},Also on line 329, remove the extra variables that // Change from:
{
request: {
query: ORGANIZATION_LIST,
variables: {
first: 8,
skip: 0,
filter: '',
orderBy: 'createdAt_ASC',
},
notifyOnNetworkStatusChange: true,
},
// To:
{
request: {
query: ORGANIZATION_FILTER_LIST,
variables: { filter: '' },
},Why This Matters
Additional Observations✅ Good: The component change in Once these changes are made to 🏁 Script executed: Length of output: 0 Review status has been set to "Changes Requested" as per your instructions. SummaryStatus: 🔴 Changes Required Issue Found: Test mock file Impact: This will cause test failures because Apollo MockedProvider won't be able to match the component's Files Requiring Changes:
The fix is straightforward and detailed in my review comment above. Once the author updates the mock file to use 🧠 Learnings used |
palisadoes
left a comment
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.
Please apply this change
|
@coderabbitai full review and reevaluate all resolved items. If the PR requires any changes, even trivial ones, then give your review a “requested changes” status. If no changes are required then give the PR an “approved” status. |
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.
Almost there! One remaining issue in OrgListMocks.ts needs to be fixed - see detailed comment.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10729 🏁 Script executed: Length of output: 169 🏁 Script executed: Length of output: 7341 🏁 Script executed: Length of output: 1643 🏁 Script executed: Length of output: 140 🏁 Script executed: Length of output: 0 @palisadoes, I've completed the full review. Great progress! The PR author has addressed most of the issues I previously identified. However, there is one remaining issue that needs to be fixed. ✅ What's FixedThe PR author has successfully updated:
❌ Remaining IssueFile: The second mock in Current code (incorrect): {
request: {
query: ORGANIZATION_FILTER_LIST,
variables: {
first: 8,
skip: 0,
filter: '',
orderBy: 'createdAt_ASC',
},
notifyOnNetworkStatusChange: true,
},
result: {
data: {
organizations: organizations,
},
},
},Should be: {
request: {
query: ORGANIZATION_FILTER_LIST,
variables: { filter: '' },
},
result: {
data: {
organizations: organizations,
},
},
},Why This MattersThe query OrganizationFilterList($filter: String) {
organizations(filter: $filter) {
...OrgFields
isMember
}
}It only accepts the Action RequiredRemove the extra variables from the mock at line 328-340 in Once this is fixed, the PR will properly solve the rate limiting issue while maintaining passing tests! 🎯 Review Status: 🔴 Changes Requested 🧠 Learnings used |
|
@coderabbitai full review and reevaluate all resolved items. If the PR requires any changes, even trivial ones, then give your review a “requested changes” status. If no changes are required then give the PR an “approved” status. |
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.
All issues have been resolved! The PR correctly fixes the rate limiting issue by switching to ORGANIZATION_FILTER_LIST, and all test mocks have been properly updated to match the query signature. Great work!
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14089 🏁 Script executed: Length of output: 7216 🏁 Script executed: Length of output: 891 🏁 Script executed: Length of output: 0 @palisadoes, excellent news! 🎉 ✅ All Issues Resolved - PR ApprovedI've completed a comprehensive review of PR #4955 and all previously identified issues have been fixed. The PR has been approved. What Was FixedThe critical issue in Before (incorrect): {
request: {
query: ORGANIZATION_LIST,
variables: {
first: 8,
skip: 0,
filter: '',
orderBy: 'createdAt_ASC',
},
notifyOnNetworkStatusChange: true,
},
...
}After (correct): {
request: {
query: ORGANIZATION_FILTER_LIST,
variables: { filter: '' },
},
...
}Summary of Changes✅ OrgList.tsx - Correctly uses
Why This Matters
The PR is ready to merge! 🚀 Review Status: ✅ Approved 🧠 Learnings used |
What kind of change does this PR introduce?
This pr fixes Search organization getting rate limited in admin portal by using a lightWeight query which does not have any unNecessary fields.
Issue Number: #4954
Fixes #4954
Snapshots/Videos:
No visual change other than user does does not gets rate limited often.
If relevant, did you update the documentation?
Auto docs updated it automatically
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Bug Fixes
Behavioral Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.