Feature/pat full analysis#89
Conversation
github-actions
Bot
added
no-issue-linked
Walkthrough
ChangesPAT-aware pagination improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
github-actions
Bot
added
external-contributor
github-actions
Bot
added
size/S
![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/context/AppContext.jsx (1)
76-83: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not publish a silently partial full-org audit.
With PAT, this can issue at least one request per repo, plus pagination. Large orgs can exceed the PAT rate budget, and
Promise.allSettledthen swallowsRATE_LIMITrejections whilesetIssuesData(map)still marks the audit complete.Add a request budget/cap or stop on
RATE_LIMITand surface the partial-audit state before expanding to all repos.🤖 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/context/AppContext.jsx` around lines 76 - 83, The batched fetch in AppContext’s repo loop can silently complete a partial org audit because Promise.allSettled ignores RATE_LIMIT failures while setIssuesData(map) still finalizes the result. Update the fetch flow around the repos batching logic to enforce a request budget/cap or stop processing as soon as fetchIssues reports RATE_LIMIT, and surface an explicit partial-audit state instead of marking completion. Use the existing symbols fetchIssues, Promise.allSettled, and setIssuesData in AppContext.jsx to locate and adjust the control flow.
🤖 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/context/AppContext.jsx`:
- Around line 39-45: In AppContext’s contributor-loading loop, the PAT branch
for computing top repositories drops the empty-array fallback, so a failed org
can leave reposPerOrg[org.login] unset and make top.map crash. Update the top
assignment in the validOrgs loop to always fall back to an empty array for both
PAT and non-PAT cases, then keep the existing sort/slice logic before
Promise.allSettled.
---
Outside diff comments:
In `@src/context/AppContext.jsx`:
- Around line 76-83: The batched fetch in AppContext’s repo loop can silently
complete a partial org audit because Promise.allSettled ignores RATE_LIMIT
failures while setIssuesData(map) still finalizes the result. Update the fetch
flow around the repos batching logic to enforce a request budget/cap or stop
processing as soon as fetchIssues reports RATE_LIMIT, and surface an explicit
partial-audit state instead of marking completion. Use the existing symbols
fetchIssues, Promise.allSettled, and setIssuesData in AppContext.jsx to locate
and adjust the control flow.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f0c39f7-01eb-4d20-8048-7711b8d2d4fa
📒 Files selected for processing (2)
src/context/AppContext.jsxsrc/services/github.js
github-actions
Bot
added
size/S
github-actions
Bot
added
size/S
|
So now are we able to fetch more repos? I just searched Microsoft and it showed me 500 repos and now with this feature we are able to fetch all of the repos of Microsoft or just some portion of it? |
Yes. Before this PR, repo fetching was limited to 500 repos regardless of organization size. With a PAT, the fetch is now dynamic and continues until all repo pages have been retrieved. For organizations like Microsoft, We can analyze all accessible repositories instead of only the first 500. The previous cap is still preserved for unauthenticated users to remain within GitHub's rate limits. |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
github-actions
Bot
added
size/S
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/context/AppContext.jsx (1)
117-118: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftPAT branch audits the full repo list, contradicting the UI and risking rate-limit exhaustion.
With a PAT,
reposbecomes the entiremodel.allRepos. For a large org (e.g. Microsoft ~8,122 repos)runAuditthen issuesfetchIssuesfor every repo in batches of 5, and eachfetchIssuesnow paginates over all issue pages — potentially tens of thousands of requests that can blow through the 5,000 req/hr PAT limit and stall the audit.It also contradicts the user-facing copy in
src/pages/AnalyticsPage.jsx("Fetching issue and PR history for top 15 repositories...") and therepoNames = ['All', ...model.allRepos.slice(0, 12)...]selector, so most fetched repos aren't even selectable.Consider capping the PAT branch to a sane bound (and/or surfacing progress), instead of unbounded
model.allRepos.⚙️ Example bound
- const repos = pat? model.allRepos : model.allRepos.slice(0, 15) + const repos = model.allRepos.slice(0, pat ? 50 : 15)🤖 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/context/AppContext.jsx` around lines 117 - 118, The PAT path in AppContext’s repo selection is using the full model.allRepos list, which can trigger unbounded audits. Update the repos assignment in AppContext.jsx so the pat branch also applies a reasonable cap (matching the UI’s top-repos behavior, e.g. the same limit used for non-PAT or selector copy), and ensure runAudit only receives that bounded list instead of all repos.src/services/github.js (1)
89-120: 🩺 Stability & Availability | 🟡 MinorVerify safety bounds in
fetchContributorsandfetchIssuespagination logic.While callers wrap these functions in
Promise.allSettled(preventing global crashes),fetchContributorsandfetchIssuesrely solely on page size to terminate the loop (for(;;) ... if(data.length < 100) break).
- Missing safety cap: Unlike
fetchRepos, these functions lack amaxPageslimit. While GitHub API usually returns an empty array on the final page, relying solely on this behavior risks infinite loops or excessive requests if the API returns partial data unexpectedly.- Error handling:
fetchWithCachethrows on 403/404 rather than returning an empty list. Ensure thePromise.allSettledusage in the caller is sufficient to handleresult.reasonfor partial failures, or consider a tighter loop bound.🤖 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/services/github.js` around lines 89 - 120, `fetchContributors` and `fetchIssues` currently use unbounded pagination loops based only on `data.length < 100`, so add a safety cap similar to `fetchRepos` by introducing a `maxPages` limit in each function and stopping when that limit is reached. Keep the pagination behavior in `fetchContributors` and `fetchIssues` tied to `fetchWithCache`, but ensure any thrown 403/404 errors remain handled by the existing `Promise.allSettled` caller by surfacing failures normally rather than looping indefinitely.
🤖 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.
Outside diff comments:
In `@src/context/AppContext.jsx`:
- Around line 117-118: The PAT path in AppContext’s repo selection is using the
full model.allRepos list, which can trigger unbounded audits. Update the repos
assignment in AppContext.jsx so the pat branch also applies a reasonable cap
(matching the UI’s top-repos behavior, e.g. the same limit used for non-PAT or
selector copy), and ensure runAudit only receives that bounded list instead of
all repos.
In `@src/services/github.js`:
- Around line 89-120: `fetchContributors` and `fetchIssues` currently use
unbounded pagination loops based only on `data.length < 100`, so add a safety
cap similar to `fetchRepos` by introducing a `maxPages` limit in each function
and stopping when that limit is reached. Keep the pagination behavior in
`fetchContributors` and `fetchIssues` tied to `fetchWithCache`, but ensure any
thrown 403/404 errors remain handled by the existing `Promise.allSettled` caller
by surfacing failures normally rather than looping indefinitely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c3ad2c2-ab32-4ba6-b10f-c12170f57db1
📒 Files selected for processing (2)
src/context/AppContext.jsxsrc/services/github.js
4 participants
Addressed Issues:
Fixes #
Screenshots/Recordings:
WITH_PAT || WITHOUT_PAT
Additional Notes:
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes