-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(selectors): gdrive and slack selectors inf loops #1376
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
Conversation
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.
Greptile Summary
This pull request fixes critical infinite loop bugs in two selector components - the Google Drive picker and Slack channel selector. Both components were experiencing continuous retry loops when encountering error conditions.
For the Google Drive picker, the fix addresses two scenarios that caused infinite retries:
- Invalid credential persistence: When a previously stored
credentialId
was no longer valid (deleted/revoked), the component would continuously attempt to use it. The fix adds validation to clear theselectedCredentialId
when it's not found in the fetched credentials list. - 401 authentication errors: When API calls returned 401 (unauthorized), the component would keep retrying the same failed request without prompting re-authentication. The fix handles 401 responses by clearing the selection (
setSelectedFileId('')
,onChange('')
,onFileInfoChange?.(null)
) and showing the OAuth modal (setShowOAuthModal(true)
).
For the Slack channel selector, the issue was in the error handling flow where initialFetchDone
was only set to true
on successful responses, never on failures. This caused the useEffect
hook to continuously trigger fetchChannels()
because the condition !initialFetchDone
remained true for failed requests. The fix ensures initialFetchDone = true
is set in all error paths:
- HTTP errors (non-ok responses)
- API error responses (when
data.error
exists) - Exception handling in the catch block
These changes align with the existing error handling patterns in the codebase and provide proper user feedback pathways for authentication and error scenarios. The fixes prevent resource waste from continuous API retries and improve user experience by offering clear recovery paths.
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it fixes clear bugs without introducing new functionality
- Score reflects well-targeted bug fixes that address specific infinite loop scenarios with proper error handling
- No files require special attention as the changes are straightforward error handling improvements
2 files reviewed, 1 comment
if (credentialId && !data.credentials.some((c: any) => c.id === credentialId)) { | ||
setSelectedCredentialId('') | ||
} |
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.
style: Type assertion to any
should be avoided. Consider defining a proper type for the credential object.
if (credentialId && !data.credentials.some((c: any) => c.id === credentialId)) { | |
setSelectedCredentialId('') | |
} | |
if (credentialId && !data.credentials.some((c: Credential) => c.id === credentialId)) { | |
setSelectedCredentialId('') | |
} |
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
Summary
Google Drive: Effect kept retrying on 401 because it didn't clear the selection → Fixed by clearing selection and showing OAuth modal on 401.
Slack: Effect kept retrying on error because initialFetchDone was never set to true on failures → Fixed by always setting initialFetchDone = true regardless of success/failure.
Type of Change
Testing
Manually
Checklist