Fix misleading private repository message during partial fetch#301
Fix misleading private repository message during partial fetch#301jagriti-aswal wants to merge 2 commits into
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthrough
ChangesPartial Success Fetch and Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useGitHubData.ts (1)
62-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove or refactor unreachable error handling code.
With
Promise.allSettled, this catch block is now mostly unreachable.Promise.allSettledalways resolves (never throws), so the specific error handling for 403 rate limiting (lines 64-66), user not found (lines 67-68), validation failures (lines 70-72), permission errors (lines 73-74), and 404 errors (lines 75-77) will never execute under normal circumstances.The only way this catch block executes now is if:
getOctokit()throws synchronously (before thePromise.allSettled)- There's a code error in the try block itself
This makes the newly added validation error handling (lines 70-72) and reordered permission/404 logic (lines 73-77) ineffective.
Either:
- Remove this dead code after moving specific error handling to inspect rejection reasons (as suggested in the previous comment), OR
- Keep only the generic fallback handler for unexpected synchronous errors
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useGitHubData.ts` around lines 62 - 83, The catch block in useGitHubData is handling specific API errors even though the fetches use Promise.allSettled (which never throws), so those branches (403/rate limit, "do not exist", "validation failed", 401/permission, 404) are effectively dead; either remove them and keep only a generic fallback for synchronous failures (e.g., errors thrown by getOctokit or other runtime exceptions), or move the specific logic into the Promise.allSettled result inspection (check each settledResult.reason or value to setRateLimited, setError('User not found'), handle validation/permission/404 cases) so error-specific messages are derived from the settled promises rather than this catch; update useGitHubData accordingly and keep getOctokit-related synchronous error handling in the catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useGitHubData.ts`:
- Around line 39-59: In useGitHubData, Promise.allSettled now swallows throw
behavior so inspect the settled results' reason fields (issueRes.reason and
prRes.reason) to restore specific handling: if either reason.status === 403 call
rateLimited(true) and set an appropriate PAT prompt, if either is
401/404/validation use those codes to set precise error messages, and if both
are rejected treat it as a complete failure (setError to indicate total failure)
vs. a partial failure when only one rejected; update the logic around
setIssues/setPrs, setTotalIssues/setTotalPrs accordingly so fulfilled results
are used while rejected ones are examined for their .reason to decide
rateLimited(), specific error text, or generic error.
---
Outside diff comments:
In `@src/hooks/useGitHubData.ts`:
- Around line 62-83: The catch block in useGitHubData is handling specific API
errors even though the fetches use Promise.allSettled (which never throws), so
those branches (403/rate limit, "do not exist", "validation failed",
401/permission, 404) are effectively dead; either remove them and keep only a
generic fallback for synchronous failures (e.g., errors thrown by getOctokit or
other runtime exceptions), or move the specific logic into the
Promise.allSettled result inspection (check each settledResult.reason or value
to setRateLimited, setError('User not found'), handle validation/permission/404
cases) so error-specific messages are derived from the settled promises rather
than this catch; update useGitHubData accordingly and keep getOctokit-related
synchronous error handling in the catch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02b64201-2464-4d71-82e2-9643eb92ca84
📒 Files selected for processing (1)
src/hooks/useGitHubData.ts
| const [issueRes, prRes] = await Promise.allSettled([ | ||
| fetchPaginated(octokit, username, 'issue', page, perPage), | ||
| fetchPaginated(octokit, username, 'pr', page, perPage), | ||
| ]); | ||
|
|
||
| setIssues(issueRes.items); | ||
| setPrs(prRes.items); | ||
| setTotalIssues(issueRes.total); | ||
| setTotalPrs(prRes.total); | ||
| if (issueRes.status === 'fulfilled') { | ||
| setIssues(issueRes.value.items); | ||
| setTotalIssues(issueRes.value.total); | ||
| } | ||
|
|
||
| if (prRes.status === 'fulfilled') { | ||
| setPrs(prRes.value.items); | ||
| setTotalPrs(prRes.value.total); | ||
| } | ||
|
|
||
| if ( | ||
| issueRes.status === 'rejected' || | ||
| prRes.status === 'rejected' | ||
| ) { | ||
| setError('Some GitHub data could not be fetched completely.'); | ||
| } |
There was a problem hiding this comment.
Inspect rejection reasons to preserve specific error handling and fix rate limiting detection.
The switch to Promise.allSettled prevents specific error details from reaching the catch block. Critical issues:
-
Rate limiting detection is broken: Line 61 always sets
rateLimited(false), but the catch block's 403 handling (lines 64-66) is now unreachable sincePromise.allSettlednever throws. Users hitting rate limits won't be prompted to add a PAT. -
Misleading error when both requests fail: When both
issueResandprResare rejected, "Some GitHub data could not be fetched completely" incorrectly implies partial success. It should indicate complete failure. -
Loss of specific error context: Rejected promises contain error details in their
reasonproperty (403 rate limit, 401 permission, 404 not found, validation errors), but these aren't inspected.
🔧 Proposed fix to inspect rejection reasons
const [issueRes, prRes] = await Promise.allSettled([
fetchPaginated(octokit, username, 'issue', page, perPage),
fetchPaginated(octokit, username, 'pr', page, perPage),
]);
+// Check for rate limiting or critical errors in rejections
+const rejections = [issueRes, prRes].filter(r => r.status === 'rejected');
+for (const rejection of rejections) {
+ const err: any = rejection.reason;
+ if (err.status === 403) {
+ setError('GitHub API rate limit exceeded. Please provide a PAT to continue.');
+ setRateLimited(true);
+ setLoading(false);
+ return;
+ }
+}
+
if (issueRes.status === 'fulfilled') {
setIssues(issueRes.value.items);
setTotalIssues(issueRes.value.total);
}
if (prRes.status === 'fulfilled') {
setPrs(prRes.value.items);
setTotalPrs(prRes.value.total);
}
-if (
- issueRes.status === 'rejected' ||
- prRes.status === 'rejected'
-) {
- setError('Some GitHub data could not be fetched completely.');
+// Set appropriate error message based on success/failure mix
+const bothFailed = issueRes.status === 'rejected' && prRes.status === 'rejected';
+const partialFailure = issueRes.status === 'rejected' || prRes.status === 'rejected';
+
+if (bothFailed) {
+ // Extract specific error message from one of the rejections
+ const err: any = issueRes.reason || prRes.reason;
+ const errorMessage = err.message?.toLowerCase() || "";
+
+ if (errorMessage.includes("do not exist")) {
+ setError('User not found. Please check the spelling of the GitHub username.');
+ } else if (errorMessage.includes("validation failed")) {
+ setError('Invalid GitHub username or insufficient permissions.');
+ } else if (err.status === 401 || errorMessage.includes("permission")) {
+ setError('Private repository detected. Please input PAT.');
+ } else if (err.status === 404) {
+ setError('Resource not found.');
+ } else {
+ setError('Unable to fetch GitHub data. Please verify the username, token, or network connection.');
+ }
+} else if (partialFailure) {
+ setError('Some GitHub data could not be fetched completely.');
}
setRateLimited(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [issueRes, prRes] = await Promise.allSettled([ | |
| fetchPaginated(octokit, username, 'issue', page, perPage), | |
| fetchPaginated(octokit, username, 'pr', page, perPage), | |
| ]); | |
| setIssues(issueRes.items); | |
| setPrs(prRes.items); | |
| setTotalIssues(issueRes.total); | |
| setTotalPrs(prRes.total); | |
| if (issueRes.status === 'fulfilled') { | |
| setIssues(issueRes.value.items); | |
| setTotalIssues(issueRes.value.total); | |
| } | |
| if (prRes.status === 'fulfilled') { | |
| setPrs(prRes.value.items); | |
| setTotalPrs(prRes.value.total); | |
| } | |
| if ( | |
| issueRes.status === 'rejected' || | |
| prRes.status === 'rejected' | |
| ) { | |
| setError('Some GitHub data could not be fetched completely.'); | |
| } | |
| const [issueRes, prRes] = await Promise.allSettled([ | |
| fetchPaginated(octokit, username, 'issue', page, perPage), | |
| fetchPaginated(octokit, username, 'pr', page, perPage), | |
| ]); | |
| // Check for rate limiting or critical errors in rejections | |
| const rejections = [issueRes, prRes].filter(r => r.status === 'rejected'); | |
| for (const rejection of rejections) { | |
| const err: any = rejection.reason; | |
| if (err.status === 403) { | |
| setError('GitHub API rate limit exceeded. Please provide a PAT to continue.'); | |
| setRateLimited(true); | |
| setLoading(false); | |
| return; | |
| } | |
| } | |
| if (issueRes.status === 'fulfilled') { | |
| setIssues(issueRes.value.items); | |
| setTotalIssues(issueRes.value.total); | |
| } | |
| if (prRes.status === 'fulfilled') { | |
| setPrs(prRes.value.items); | |
| setTotalPrs(prRes.value.total); | |
| } | |
| // Set appropriate error message based on success/failure mix | |
| const bothFailed = issueRes.status === 'rejected' && prRes.status === 'rejected'; | |
| const partialFailure = issueRes.status === 'rejected' || prRes.status === 'rejected'; | |
| if (bothFailed) { | |
| // Extract specific error message from one of the rejections | |
| const err: any = issueRes.reason || prRes.reason; | |
| const errorMessage = err.message?.toLowerCase() || ""; | |
| if (errorMessage.includes("do not exist")) { | |
| setError('User not found. Please check the spelling of the GitHub username.'); | |
| } else if (errorMessage.includes("validation failed")) { | |
| setError('Invalid GitHub username or insufficient permissions.'); | |
| } else if (err.status === 401 || errorMessage.includes("permission")) { | |
| setError('Private repository detected. Please input PAT.'); | |
| } else if (err.status === 404) { | |
| setError('Resource not found.'); | |
| } else { | |
| setError('Unable to fetch GitHub data. Please verify the username, token, or network connection.'); | |
| } | |
| } else if (partialFailure) { | |
| setError('Some GitHub data could not be fetched completely.'); | |
| } | |
| setRateLimited(false); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useGitHubData.ts` around lines 39 - 59, In useGitHubData,
Promise.allSettled now swallows throw behavior so inspect the settled results'
reason fields (issueRes.reason and prRes.reason) to restore specific handling:
if either reason.status === 403 call rateLimited(true) and set an appropriate
PAT prompt, if either is 401/404/validation use those codes to set precise error
messages, and if both are rejected treat it as a complete failure (setError to
indicate total failure) vs. a partial failure when only one rejected; update the
logic around setIssues/setPrs, setTotalIssues/setTotalPrs accordingly so
fulfilled results are used while rejected ones are examined for their .reason to
decide rateLimited(), specific error text, or generic error.
|
After further debugging, I found that the displayed PR data was coming from stale frontend state rather than a successful partial GitHub fetch. The issue is therefore different from the originally reported behavior. I identified the actual problem as stale state persistence and improved fetch/error handling accordingly. I’ll create a separate issue and PR focused on the correct root cause. |
1 participant
Related Issue
Closes #300
Description
This PR improves GitHub tracker error handling during partial API fetch failures.
Changes made:
Promise.all()withPromise.allSettled()How Has This Been Tested?
Screenshots
Before
After
Added in PR discussion.
Type Of Change
Summary by CodeRabbit
Release Notes