Website: Combine the fetch logic in open-source page #985
Conversation
activus-d
requested review from
andrashee,
mkue,
ssandino and
socialincome-dev
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a centralized Changes
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts (1)🔇 Additional comments (3)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts (3)Line range hint The simplified interface structure improves code readability by directly mapping to the API response format. Line range hint The implementation includes proper validation and fallback handling for edge cases.
The change from ✅ Verification successfulAPI endpoint change appears to be an improvement The switch from
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Compare the response format and data between old and new endpoints
# Test both endpoints
curl -s -H "Accept: application/vnd.github.v3+json" \
"https://api.github.com/repos/socialincome-san/public/contributors" | \
jq 'map({id, login, contributions, avatar_url}) | .[0:2]'
curl -s -H "Accept: application/vnd.github.v3+json" \
"https://api.github.com/repos/socialincome-san/public/stats/contributors" | \
jq '.[0:2]'
Length of output: 677 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-forks.ts (1)
Line range hint
29-39: Optimize pagination and date comparisonThe current implementation could hit rate limits with many pages. Consider optimizing the pagination logic.
- while (hasMore) { + const maxPages = 10; // Limit pages to prevent excessive API calls + while (hasMore && page <= maxPages) { const pagedRes = await fetchData(owner, repo, `${forksUrl}&page=${page}`); const forks: GitHubFork[] = await pagedRes.json(); + const startDateTime = new Date(startDateISO).getTime(); // Count new forks within the last 30 days for (const fork of forks) { - if (new Date(fork.created_at) >= new Date(startDateISO)) { + if (new Date(fork.created_at).getTime() >= startDateTime) { newForks++; + } else { + hasMore = false; // Stop if we hit older forks + break; } }
🧹 Nitpick comments (4)
website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts (3)
1-2: Remove redundant variable assignmentThe
commitUrlvariable is unnecessary since theurlparameter is already available.export async function fetchData(owner: string, repo: string, url: string) { - const commitUrl = url;
3-9: Enhance headers configurationAdd GitHub API version header and improve type safety.
- const headers: Record<string, string> = { + const headers = { Accept: 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', }; - if (process.env.GITHUB_PAT) { + if (process.env.GITHUB_PAT?.trim()) { headers['Authorization'] = `Bearer ${process.env.GITHUB_PAT}`; }
1-29: Add rate limiting protectionConsider implementing request throttling to prevent hitting GitHub API rate limits.
Consider adding a simple rate limiter:
const rateLimit = { requests: 0, resetTime: Date.now() + 3600000, // 1 hour window limit: 5000 // GitHub's default limit }; function checkRateLimit() { if (Date.now() > rateLimit.resetTime) { rateLimit.requests = 0; rateLimit.resetTime = Date.now() + 3600000; } if (rateLimit.requests >= rateLimit.limit) { throw new Error('Local rate limit exceeded'); } rateLimit.requests++; }website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts (1)
1-1: Consider error handling strategy for centralized fetchWhile centralizing fetch logic improves maintainability, ensure that
fetchDataprovides adequate error differentiation for different API endpoints and use cases.Consider adding specific error types or status code handling in
fetchDatato help consumers handle different error scenarios appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts(1 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-forks.ts(3 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts(3 hunks)
🧰 Additional context used
📓 Learnings (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts (1)
Learnt from: activus-d
PR: socialincome-san/public#926
File: website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.tsx:51-85
Timestamp: 2024-11-29T07:52:05.401Z
Learning: In `website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.tsx`, the function `getStarCount` does not require optimization to stop pagination when older stars are encountered, as this optimization is not relevant.
🔇 Additional comments (2)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts (1)
33-33: LGTM! Pagination implementation is correct
The URL construction and page parameter handling is properly implemented within the loop.
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts (1)
Line range hint 33-46: Verify Link header parsing behavior
The Link header parsing logic includes good error handling and fallbacks. However, let's verify the behavior across different scenarios.
✅ Verification successful
Link header parsing implementation is correct and robust
The current implementation correctly handles GitHub's Link header format. The test confirms that:
- The regex pattern
&page=(\d+)>; rel="last"matches the actual GitHub API response format - The fallback to 1 is appropriate for cases where the header might be missing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test Link header parsing with different response patterns
curl -I "https://api.github.com/repos/socialincome-san/public/commits?per_page=1" \
-H "Accept: application/vnd.github.v3+json" | grep "link:"
Length of output: 638
website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts
Show resolved
Hide resolved
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts
Outdated
Show resolved
Hide resolved
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts (1)
Line range hint
1-47: Consider caching for API rate limit optimizationWhile the current implementation is solid, consider implementing caching for the star data to reduce API calls and handle rate limits more effectively.
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts (2)
3-4: Consider moving repository details to configurationThe hardcoded repository details limit reusability. Consider moving these to environment variables or configuration.
-const owner = 'socialincome-san'; -const repo = 'public'; +const owner = process.env.GITHUB_OWNER || 'socialincome-san'; +const repo = process.env.GITHUB_REPO || 'public';
Line range hint
33-47: Improve Link header parsing robustnessThe current Link header parsing is fragile. Consider using a dedicated parser for Link headers.
+function parseLinkHeader(header: string | null): number { + if (!header) return 1; + const links = header.split(','); + const lastLink = links.find(link => link.includes('rel="last"')); + if (!lastLink) return 1; + const pageMatch = lastLink.match(/[&?]page=(\d+)/); + return pageMatch ? parseInt(pageMatch[1], 10) : 1; +} -const linkHeader = totalCommitsRes.headers.get('link'); -let totalCommits = 1; - -if (linkHeader) { - const match = linkHeader.match(/&page=(\d+)>; rel="last"/); - if (match) { - totalCommits = parseInt(match[1], 10); - } else { - console.warn('Link header exists but could not parse total commits page count.'); - } -} else { - console.warn('No Link header found; assuming a single commit.'); -} +const totalCommits = parseLinkHeader(totalCommitsRes.headers.get('link'));website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts (1)
Line range hint
27-31: Enhance error logging for debuggingConsider adding more context to the warning message, such as the URL and timestamp, to aid in debugging.
if (Object.keys(contributors).length === 0) { - console.warn('No contributor data available. The API returned an empty object.'); + console.warn(`No contributor data available from ${url} at ${new Date().toISOString()}`); return []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts(1 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-forks.ts(3 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts
- website/src/app/[lang]/[region]/(website)/open-source/(components)/get-forks.ts
- website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts
🧰 Additional context used
📓 Learnings (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts (1)
Learnt from: activus-d
PR: socialincome-san/public#926
File: website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.tsx:51-85
Timestamp: 2024-11-29T07:52:05.401Z
Learning: In `website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.tsx`, the function `getStarCount` does not require optimization to stop pagination when older stars are encountered, as this optimization is not relevant.
🔇 Additional comments (6)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts (3)
1-1: LGTM! Import of centralized fetch utility
The addition of the fetchData utility aligns with the PR's goal of consolidating fetch logic.
33-33: LGTM! Consistent use of fetchData utility
The pagination logic remains sound while benefiting from centralized fetch handling.
16-16: Verify error handling in fetchData utility
Ensure the fetchData utility properly handles API rate limits and authentication errors.
✅ Verification successful
Error handling is properly implemented in fetchData utility
The fetchData utility in website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts implements comprehensive error handling for:
- Authentication errors (403)
- Not found errors (404)
- Other API failures with detailed error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if fetchData implements proper error handling
ast-grep --pattern 'export async function fetchData($$$) {
$$$
if ($response.status === 403 || $response.status === 404) {
$$$
}
$$$
}'
Length of output: 149
Script:
#!/bin/bash
# Let's first locate the fetchData implementation
fd fetchData
Length of output: 14
Script:
#!/bin/bash
# Search for error handling patterns in files containing fetchData
rg -A 5 "export.*fetchData"
Length of output: 777
Script:
#!/bin/bash
# Look for any error handling patterns in files containing fetchData
rg "catch|throw|Error" $(fd fetchData)
Length of output: 21981
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts (1)
1-1: Verify fetchData implementation
Let's ensure the fetchData function is properly implemented with the expected signature.
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts (2)
22-25: Add retry logic for empty responses
The GitHub contributors stats API may return an empty response while computing stats. The retry logic suggestion from the previous review is still applicable.
1-1: Verify the fetch-data module exists
✅ Verification successful
Import path './fetch-data' is correctly configured
The fetch-data.ts module exists in the same directory as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the fetch-data module exists in the same directory
fd -e ts "fetch-data.ts" "website/src/app"
Length of output: 121
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts (1)
3-4: Consider making repository details configurableThe owner and repo values are hardcoded, which limits the reusability of this function for other repositories.
-const owner = 'socialincome-san'; -const repo = 'public'; +const GITHUB_OWNER = process.env.GITHUB_OWNER || 'socialincome-san'; +const GITHUB_REPO = process.env.GITHUB_REPO || 'public';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts(2 hunks)
🔇 Additional comments (2)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts (2)
25-26: Fix URL construction for pagination
The URL with the page parameter is constructed outside the loop but used inside it. This means the page number won't update correctly during pagination.
- const url = `https://api.github.com/repos/${owner}/${repo}/issues?state=open&per_page=100&page=${page}`;
while (hasMore) {
+ const url = `https://api.github.com/repos/${owner}/${repo}/issues?state=open&per_page=100&page=${page}`;
const res = await fetchData(owner, repo, url);Line range hint 27-58: LGTM! Well-structured data processing
The implementation demonstrates good practices:
- Proper type handling with interfaces
- Efficient filtering and mapping of issues
- Smart label deduplication
- Clear pagination logic
activus-d
changed the title
…logic in the open-source page
activus-d
force-pushed
the
combine-fetch-logic
branch
from
54eabe8 to
6249e3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts (1)
3-4: Consider parameterizing repository detailsThe hardcoded owner and repo constants limit the reusability of this component. Consider making these parameters of the
getIssuesDatafunction.-const owner = 'socialincome-san'; -const repo = 'public'; +export async function getIssuesData(owner: string, repo: string): Promise<IssuesResponse>website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts (1)
Line range hint
1-11: Consider moving repository constants to a central configuration.The
ownerandrepoconstants could be moved to a shared configuration file since they might be used across multiple components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts(1 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-forks.ts(3 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts(2 hunks)website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- website/src/app/[lang]/[region]/(website)/open-source/(components)/get-commits.ts
- website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts
- website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts
- website/src/app/[lang]/[region]/(website)/open-source/(components)/get-forks.ts
🔇 Additional comments (3)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-issues.ts (1)
25-26: LGTM! Verify fetchData implementation
The URL construction and data fetching logic look correct. The pagination implementation is sound.
Let's verify the fetchData implementation:
✅ Verification successful
fetchData implementation is robust and consistently used
The fetchData utility:
- Properly handles GitHub API authentication
- Has specific error handling for rate limits (403) and not found (404)
- Is consistently used across all GitHub API calls with proper response handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify fetchData implementation and error handling
# Check if fetchData exists and handles errors appropriately
ast-grep --pattern 'export async function fetchData($_) {
$$$
}'
# Check for consistent error handling across other fetch operations
rg -l "fetchData\(" --type ts
Length of output: 591
Script:
#!/bin/bash
# Let's check the fetchData implementation
cat website/src/app/[lang]/[region]/\(website\)/open-source/\(components\)/fetch-data.ts
# Check how it's used in other files for consistency
rg "fetchData\(" -A 2 --type ts
Length of output: 4355
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-stars.ts (2)
16-16: Verify error handling in fetchData utility.
The function now delegates error handling to fetchData. Let's verify its implementation.
✅ Verification successful
Error handling in fetchData utility is properly implemented
The fetchData utility includes comprehensive error handling:
- Rate limit exceeded (403) with clear user guidance
- Repository not found (404) with specific error message
- Generic error handling with status code and details
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check fetchData implementation for proper error handling
ast-grep --pattern 'export async function fetchData($_) {
$$$
}'
# Check for other components using similar error handling
rg -l "api.github.com/repos/${owner}/${repo}"
Length of output: 99
Script:
#!/bin/bash
# Search for fetchData function definition
rg -A 10 "export.*function fetchData"
# Search for any fetch-related utility functions
rg -A 10 "async.*fetch.*data"
# Look for files importing fetchData
rg -l "import.*fetchData"
Length of output: 1785
Script:
#!/bin/bash
# Get the complete implementation of fetchData to check error handling
cat website/src/app/[lang]/[region]/\(website\)/open-source/\(components\)/fetch-data.ts
Length of output: 943
Line range hint 33-43: Consider extracting pagination constants and handling rate limits.
The pagination size (100) could be moved to a constant. Also, verify that fetchData handles GitHub API rate limits appropriately for paginated requests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts (1)
Line range hint
25-27: Enhance empty response error messageThe warning message could be more specific about why the data might be empty.
-console.warn('No contributor data available. The API returned an empty object.'); +console.warn('No contributor data available. This could happen if the repository is empty or if there are no contributions yet.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts(3 hunks)
🧰 Additional context used
📓 Learnings (1)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts (2)
Learnt from: activus-d
PR: socialincome-san/public#985
File: website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts:22-25
Timestamp: 2024-12-16T04:00:08.419Z
Learning: In the `get-contributors.ts` file, the `getContributors()` function does not require retry logic for empty responses from the GitHub contributor stats API.
Learnt from: activus-d
PR: socialincome-san/public#926
File: website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.tsx:28-30
Timestamp: 2024-11-29T08:25:44.729Z
Learning: In the React TypeScript file `get-contributors.tsx`, adding timeout and retry logic to the `getContributors` function's GitHub API calls is not necessary.
🔇 Additional comments (2)
website/src/app/[lang]/[region]/(website)/open-source/(components)/get-contributors.ts (2)
Line range hint 1-17: LGTM! Interface changes align with GitHub API structure
The flattened GitHubContributor interface correctly matches the GitHub API response structure, and the centralized fetchData import improves code organization.
20-23: LGTM! Improved API endpoint and centralized fetch logic
The switch to /contributors endpoint and use of centralized fetchData improves reliability and maintainability.
…oint to avoid timing issue during the computation phase on github servers
activus-d
force-pushed
the
combine-fetch-logic
branch
from
9b4d7f1 to
c79c2a9
Compare
This PR combines the fetch logic in open-source page for a more concise code.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation