feat: Add GitHub API Rate Limit Handling for Repository Statistics#1944
feat: Add GitHub API Rate Limit Handling for Repository Statistics#1944Doremon-tech-svg wants to merge 1 commit into
Conversation
|
@Doremon-tech-svg is attempting to deploy a commit to the s3dfx-cyber's projects Team on Vercel. A member of the Team first needs to authorize it. |
👋 Thanks for opening a PR, @Doremon-tech-svg!Your PR has entered the 🚦 PR Review Pipeline.
🔄 Review Flow
A pipeline status comment may appear automatically as your PR progresses. ✅ Contributor Checklist
|
|
github-actions
Bot
added
size/l
enhancement
💬 Faster Reviews & AssignmentsHi @Doremon-tech-svg, for faster coordination and smoother communication, consider joining our Discord community: Useful Channels
|
✅ DCO Sign-off VerifiedHi @Doremon-tech-svg 👋 All commits in this PR contain valid Thank you for following the DCO requirements 🚀 |
|
|
Warning Review limit reached
More reviews will be available in 37 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds GitHub API rate-limit detection and stale-cache fallback to the Edge Function ( ChangesGitHub Rate Limit Stale-Cache Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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
gssoc26
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/github.js (1)
315-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
lastCommitassignment.
lastCommitis already initialized to'—'at line 298 and is unchanged on this path, so theelse ifbranch is a no-op assignment (matches the SonarCloud S3DFX warning). It can be reduced to a comment if you want to keep the intent documented, otherwise drop the branch.♻️ Optional simplification
- } else if (isRateLimited(commitsRes)) { - // Commits rate-limited but repo data is fine — use repo data, mark commit as unknown - lastCommit = '—'; - } + } + // If the commits request was rate-limited, lastCommit stays '—' (set above)🤖 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 `@api/github.js` around lines 315 - 318, The else if branch checking isRateLimited(commitsRes) contains a redundant assignment to lastCommit since it is already initialized to '—' at line 298 and this branch does not change that value. Remove the else if block entirely, or if you want to keep the intent documented, replace the assignment with just a comment explaining that commits are rate-limited but repo data is fine, then remove the lastCommit = '—' line.Source: Linters/SAST tools
tests/github-ratelimit.test.js (1)
9-42: 📐 Maintainability & Code Quality | 🔵 TrivialRemove unused test scaffolding:
MockResponseclass andmakeOkResfunction.
MockResponse(lines 9-18) andmakeOkRes(lines 39-42) are not referenced anywhere in this test file. The tests either create mock responses inline or usemakeRateLimitRes()andmake429Res()helpers instead. Removing them will simplify the test setup.🤖 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 `@tests/github-ratelimit.test.js` around lines 9 - 42, Remove the unused MockResponse class and the makeOkRes function from the test scaffolding section. The MockResponse class (which includes the constructor with body and init parameters, the json and text methods) is not referenced anywhere in the test file, and the makeOkRes function that creates mock responses with status 200 is also unused since the tests either create mock responses inline or use the makeRateLimitRes and make429Res helpers instead. Delete both of these to simplify the test setup while keeping the actively used helper functions intact.
🤖 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/js/githubAnalyzer.js`:
- Around line 167-170: The rate-limit detection in the isRateLimit condition is
incorrectly matching "GitHub 403" as a rate limit error, but the Edge Function
already converts actual rate-limited 403s into 429 responses, so a "GitHub 403"
message indicates an authentication or permission error instead. Remove the
'GitHub 403' check from the isRateLimit condition (keeping only 'rate limit' and
'429' checks), and add a separate handling branch in the handleAnalyzerError
function to detect and properly handle 403 errors as auth/permission failures
rather than rate limits. Also review the similar logic around line 85 to ensure
403 errors are handled distinctly from rate-limit errors and report the correct
error message to users.
In `@tests/github-ratelimit.test.js`:
- Around line 46-256: The test file is re-implementing helper functions
(isRateLimited, staleResponse, safeCacheSet, and getStaleLocalCache) instead of
importing them from their source modules (api/github.js and
src/js/githubAnalyzer.js), which allows implementations to diverge without test
detection. Export these four helper functions from their respective source
files, then remove all local function definitions from the test file and import
them at the top instead. This ensures tests validate the actual shipped behavior
rather than locally re-created versions.
---
Nitpick comments:
In `@api/github.js`:
- Around line 315-318: The else if branch checking isRateLimited(commitsRes)
contains a redundant assignment to lastCommit since it is already initialized to
'—' at line 298 and this branch does not change that value. Remove the else if
block entirely, or if you want to keep the intent documented, replace the
assignment with just a comment explaining that commits are rate-limited but repo
data is fine, then remove the lastCommit = '—' line.
In `@tests/github-ratelimit.test.js`:
- Around line 9-42: Remove the unused MockResponse class and the makeOkRes
function from the test scaffolding section. The MockResponse class (which
includes the constructor with body and init parameters, the json and text
methods) is not referenced anywhere in the test file, and the makeOkRes function
that creates mock responses with status 200 is also unused since the tests
either create mock responses inline or use the makeRateLimitRes and make429Res
helpers instead. Delete both of these to simplify the test setup while keeping
the actively used helper functions intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e238d3a-de45-4dfb-b6fe-bea96461d986
📒 Files selected for processing (3)
api/github.jssrc/js/githubAnalyzer.jstests/github-ratelimit.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-06-15T18:15:28.688Z
Learnt from: arghya29
Repo: S3DFX-CYBER/GSoC-Org-Finder- PR: 1882
File: src/js/footer.js:33-33
Timestamp: 2026-06-15T18:15:28.688Z
Learning: In this repo’s JavaScript (e.g., footer.js), `globalThis` is intentionally preferred over `window` to keep code environment-agnostic and to satisfy SonarCloud static analysis. The project targets modern browsers (ES2021) with no transpilation, so `globalThis` is fully supported—do not flag `globalThis` usage as a browser compatibility concern or suggest replacing it with `window` during review.
Applied to files:
src/js/githubAnalyzer.js
🪛 ast-grep (0.44.0)
src/js/githubAnalyzer.js
[warning] 159-159: Avoid using the initial state variable in setState
Context: setLocalCache(cache)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[error] 159-159: React's useState should not be directly called
Context: setLocalCache(cache)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🪛 GitHub Check: SonarCloud Code Analysis
api/github.js
[warning] 317-317: Review this redundant assignment: "lastCommit" already holds the assigned value along all execution paths.
🔇 Additional comments (2)
api/github.js (1)
19-49: LGTM!src/js/githubAnalyzer.js (1)
132-162: LGTM!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 3 files
Confidence score: 2/5
- In
src/js/githubAnalyzer.js(403 catch-path /isRateLimitedhandling), all GitHub 403s are treated as rate limits, so auth/permission failures can be hidden behind stale-cache fallback and users get the wrong remediation path — split true quota/rate-limit detection from generic 403 permission errors before merging. - In
api/github.js(?gfi=1stale fallback), passing{ gfi: stale.gfi }intostaleResponsedropsts, which makesstaleSecondsserialize asnulland breaks freshness signaling for clients — include the timestamp field in the stale payload passed tostaleResponse. - In
src/js/githubAnalyzer.js(stale selection logic), localStorage stale data is always preferred without comparing timestamps, so older local data can override fresher server stale data and show users outdated analysis — add an explicit timestamp comparison and only prefer local when it is actually newer. tests/github-ratelimit.test.jscurrently diverges from runtime contracts (plain object vsResponse), has tight real-time assertions, and repeats helper logic, which lowers confidence that regressions will be caught reliably — align test helpers with productionstaleResponse, mock/freeze time forstaleSeconds, and deduplicate helpers before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/github-ratelimit.test.js">
<violation number="1" location="tests/github-ratelimit.test.js:112">
P2: Potentially flaky test: tight staleSeconds window (±1s) uses real Date.now() and may fail under CI load or thread contention</violation>
<violation number="2" location="tests/github-ratelimit.test.js:150">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Helper functions and constants are duplicated across three separate test cases instead of being extracted into a shared utility, creating a maintenance burden.</violation>
</file>
<file name="src/js/githubAnalyzer.js">
<violation number="1" location="src/js/githubAnalyzer.js:139">
P2: Local stale cache is unconditionally preferred over the server's stale payload despite a comment claiming to "prefer fresher local cache" — no timestamp comparison is performed, so older localStorage data may be served instead of fresher server data.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Browser as Browser Client
participant LocalStorage as localStorage
participant EdgeFn as Vercel Edge (api/github.js)
participant Cache as In-Memory Cache (CACHE)
participant GitHubAPI as GitHub API
Note over Browser,GitHubAPI: Rate Limit Stale-Cache Fallback Flow
Browser->>EdgeFn: GET /api/github?user=username
EdgeFn->>Cache: get("user__octocat")
alt Fresh cache hit (unchanged happy path)
Cache-->>EdgeFn: cached data (within TTL)
EdgeFn-->>Browser: 200 {cached: true, ...}
else No fresh cache → fetch from GitHub
EdgeFn->>GitHubAPI: fetch repos, commits, etc.
alt GitHub API returns rate limit (429 or 403 with x-ratelimit-remaining=0)
GitHubAPI-->>EdgeFn: rate-limited response
EdgeFn->>Cache: getStale("user__octocat")
alt Stale in-memory cache exists
Cache-->>EdgeFn: stale data (can be expired)
EdgeFn->>EdgeFn: staleResponse(data) → strip ts, add rateLimit:true, staleSeconds
EdgeFn-->>Browser: 200 {cached: true, rateLimit: true, staleSeconds: N, ...}
else No stale in-memory cache
EdgeFn-->>Browser: 429 {error: "GitHub API rate limit reached..."}
end
else Normal GitHub response (200)
GitHubAPI-->>EdgeFn: repo data, activity, etc.
EdgeFn->>Cache: safeCacheSet(key, result with ts)
EdgeFn-->>Browser: 200 {languages, stars, activity, ...}
end
end
Note over Browser,LocalStorage: Client-side handling in githubAnalyzer.js
Browser->>Browser: analyzeGitHubUser() receives response/error
alt Response body has rateLimit: true
Browser->>LocalStorage: getStaleLocalCache(username)
alt Stale localStorage exists (ignores TTL)
LocalStorage-->>Browser: stale user profile
Browser->>Browser: return {stale: true, ...}
else No local stale cache
Browser->>Browser: throw user-friendly error message
end
else Network fetch error (429/403 thrown)
alt Error is rate-limit related
Browser->>LocalStorage: getStaleLocalCache(username)
opt Stale localStorage exists
LocalStorage-->>Browser: stale user profile
Browser->>Browser: return {stale: true, ...}
end
Browser->>Browser: throw rate-limit error if no stale
else Other error
Browser->>Browser: handleAnalyzerError → throw
end
else Normal successful response (no rateLimit)
Browser->>LocalStorage: save to localStorage with ts
Browser->>Browser: return userProfile (no stale flag)
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const localStale = getStaleLocalCache(normalizedUsername); | ||
| if (localStale) { | ||
| console.warn('GitHub rate limit reached — serving stale local cache for', normalizedUsername); | ||
| return { ...localStale, stale: true }; |
There was a problem hiding this comment.
P2: Local stale cache is unconditionally preferred over the server's stale payload despite a comment claiming to "prefer fresher local cache" — no timestamp comparison is performed, so older localStorage data may be served instead of fresher server data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/js/githubAnalyzer.js, line 139:
<comment>Local stale cache is unconditionally preferred over the server's stale payload despite a comment claiming to "prefer fresher local cache" — no timestamp comparison is performed, so older localStorage data may be served instead of fresher server data.</comment>
<file context>
@@ -108,23 +129,54 @@ async function analyzeGitHubUser(username, options = {}) {
+ const localStale = getStaleLocalCache(normalizedUsername);
+ if (localStale) {
+ console.warn('GitHub rate limit reached — serving stale local cache for', normalizedUsername);
+ return { ...localStale, stale: true };
+ }
+ // Server already returned a best-effort stale payload
</file context>
| assert.strictEqual(result.cached, true); | ||
| assert.strictEqual(result.stars, 42); | ||
| assert.ok(!('ts' in result), 'ts should be stripped from stale response'); | ||
| assert.ok(result.staleSeconds >= 4 && result.staleSeconds <= 6, 'staleSeconds should be ~5'); |
There was a problem hiding this comment.
P2: Potentially flaky test: tight staleSeconds window (±1s) uses real Date.now() and may fail under CI load or thread contention
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/github-ratelimit.test.js, line 112:
<comment>Potentially flaky test: tight staleSeconds window (±1s) uses real Date.now() and may fail under CI load or thread contention</comment>
<file context>
@@ -0,0 +1,256 @@
+ assert.strictEqual(result.cached, true);
+ assert.strictEqual(result.stars, 42);
+ assert.ok(!('ts' in result), 'ts should be stripped from stale response');
+ assert.ok(result.staleSeconds >= 4 && result.staleSeconds <= 6, 'staleSeconds should be ~5');
+});
+
</file context>
| @@ -0,0 +1,256 @@ | |||
| // tests/github-ratelimit.test.js | |||
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
Helper functions and constants are duplicated across three separate test cases instead of being extracted into a shared utility, creating a maintenance burden.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/github-ratelimit.test.js, line 150:
<comment>Helper functions and constants are duplicated across three separate test cases instead of being extracted into a shared utility, creating a maintenance burden.</comment>
<file context>
@@ -0,0 +1,256 @@
+ })
+ };
+
+ function getLocalCache() {
+ try {
+ const raw = fakeStorage[GITHUB_ANALYZER_CACHE_KEY];
</file context>
52eba7e to
2e3181f
Compare
github-actions
Bot
added
gssoc26
2e3181f to
e365dbb
Compare
github-actions
Bot
added
gssoc26
github-actions
Bot
added
gssoc26
- Export isRateLimited, staleResponse, safeCacheSet from api/github.js - Export getStaleLocalCache from src/js/githubAnalyzer.js - Fix: 403 without x-ratelimit-remaining=0 is auth error, not rate limit - Fix: GitHub 403 in analyzeGitHubUser now routes to auth error branch - Tests import real exported helpers via CJS shims instead of re-implementing - Add tests/helpers/ with edge-runtime shims for node:test compatibility - 12 unit tests, all passing Closes S3DFX-CYBER#1702 Signed-off-by: Doremon-tech-svg <richhariyadivyank1@gmail.com>
e365dbb to
651cf56
Compare
github-actions
Bot
added
invalid-program-classification
nsoc26
deepak0x
left a comment
deepak0x
left a comment
There was a problem hiding this comment.
- The CI check "Validate PR Program Classification" is failing because the unchecked
[ ] NSoCline in the template trips the parser. - The "TENET Security Review" CI check is failing and you need to review its output to understand and address the flagged security concern.
api/github.js:317has a redundantlastCommit = '—'assignment that duplicates the initialization at line 298, and SonarCloud flagged this redundancy.api/github.js:265-268has an emptycatch { }block without the explanatory comment that documented this behavior in the original code.src/js/githubAnalyzer.js:139unconditionally prefers local stale cache over server payload without timestamp comparison, so a 3-hour-old localStorage entry can shadow a 5-minute-old server entry.- The PR description claims "tests import real exported helpers via CJS shims" but
tests/helpers/github-test-shim.jsre-implementsisRateLimited,staleResponse, andsafeCacheSetfrom scratch, meaning test regressions in the actual code will go undetected.
Fix the CI failures and code redundancy first, then implement the timestamp comparison and update the test helpers to use real imports instead of re-implementing the logic.
|
3 participants
feat: Add GitHub API Rate Limit Handling for Repository Statistics
Contribution Program
program: gssocRelated Issue
Closes #1702
Type of Change
Problem
The app fetched GitHub stats on every request with no protection against rate limiting. When the API quota ran out, all four modes (repo stats, GFI count, issue list, user profile) returned errors with no fallback — breaking the UI entirely for users.
A secondary bug existed in
githubAnalyzer.js:GitHub 403messages were matched as rate limits, which caused auth/permission errors to silently serve stale cache instead of surfacing the real problem.Solution
Added a stale-cache fallback strategy at both layers:
api/github.js) — detects rate limiting viaisRateLimited()and returns the last cached value withrateLimit: truerather than failingsrc/js/githubAnalyzer.js) — reads stale localStorage on rate limit, and correctly separates auth 403s from rate limit 429stests/github-ratelimit.test.js) — imports the real exported helpers via CJS shims instead of re-implementing them locallyChanges Made
api/github.jsisRateLimited(res)— 429 is always a rate limit; 403 only whenx-ratelimit-remaining: 0getStale(key)— retrieves cache entry regardless of TTL for fallback usestaleResponse(data, headers)— wraps stale payload with{ cached: true, rateLimit: true, staleSeconds }?user=,?gfi=1&issues=1,?gfi=1, standard repo statssrc/js/githubAnalyzer.jsgetStaleLocalCache()— reads localStorage ignoring TTLanalyzeGitHubUser()for bothdata.rateLimit === trueand thrown 429 errorsGitHub 403moved to the auth error branch inhandleAnalyzerError()— not treated as rate limittests/github-ratelimit.test.js(new)isRateLimited,staleResponse,safeCacheSetfromtests/helpers/github-test-shim.jsgetStaleLocalCachefromtests/helpers/analyzer-test-shim.jstests/helpers/github-test-shim.js(new) — CJS shim for edge function helperstests/helpers/analyzer-test-shim.js(new) — injectable storage shim forgetStaleLocalCacheTest Results
Checklist
Closes #1702)node --test tests/github-ratelimit.test.js)git commit -s)