Move GitHub token server-side & add ETag conditional caching to reduce API calls #892
Conversation
Refactor GitHubService to support ETag conditional requests and improve caching. Removed client-side token reading and added methods for managing cached meta data.
|
@mukeshdhadhariya is attempting to deploy a commit to the recode Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. The estimated time for response is 5–8 hrs. In the meantime, please provide all necessary screenshots and make sure you run - npm build run , command and provide a screenshot, a video recording, or an image of the update you made below, which helps speed up the review and assignment. If you have questions, reach out to LinkedIn. Your contributions are highly appreciated!😊 Note: I maintain the repo issue every day twice at 8:00 AM IST and 9:00 PM IST. If your PR goes stale for more than one day, you can tag and comment on this same issue by tagging @sanjay-kv. We are here to help you on this journey of open source. Consistent 20 contributions are eligible for sponsorship 💰 🎁 check our list of amazing people we sponsored so far: GitHub Sponsorship. ✨ 📚Your perks for contribution to this community 👇🏻
If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
github-actions
bot
added
hacktoberfest-accepted
level 1
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@Adez017 can you cross check this. |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the GitHub API service by moving authentication tokens server-side and implementing ETag-based conditional caching to reduce API calls and prevent rate limiting.
- Removes client-side token access through
window.GITHUB_TOKENand adds constructor/method-based token setting - Implements ETag conditional caching with
If-None-Matchheaders to avoid unnecessary API calls - Enhances cache structure to store ETags and metadata alongside data
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/services/githubService.ts
Outdated
| signal, | ||
| }, | ||
| ); | ||
| const url = `${this.BASE_URL}/search/issues?q=org:${this.ORG_NAME}+type:issue`; |
There was a problem hiding this comment.
The search query changed from repo-specific to org-wide (org:${this.ORG_NAME} instead of repo:${this.ORG_NAME}/Support). This will return all issues across the organization rather than discussions from a specific repository, potentially causing inaccurate discussion counts.
src/services/githubService.ts
Outdated
| }, | ||
| ); | ||
| // Use per_page=1 to leverage Link header for total pages, if present | ||
| const url = `${this.BASE_URL}/repos/${repo.full_name}/contributors?per_page=1&anon=true`; |
There was a problem hiding this comment.
Adding anon=true parameter changes the API behavior to include anonymous contributors. This may affect contributor count accuracy compared to the previous implementation and should be documented or made configurable.
src/services/githubService.ts
Outdated
| const response = await this.fetchWithRetry("https://api.github.com/graphql", { | ||
| method: "POST", | ||
| headers: { | ||
| ...this.getHeaders(), | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ query, variables }), | ||
| body: { query, variables }, | ||
| signal, | ||
| }); |
There was a problem hiding this comment.
The body should be a JSON string, but it's being passed as an object. The fetchWithRetry method calls JSON.stringify(options.body) only when options.body exists, but this will result in double stringification since the body is already an object here.
There was a problem hiding this comment.
@mukeshdhadhariya @sanjay-kv Updation of the Readme.md file remains and it is important as we are having new contributors, rest is fine :)
Also run 'npm run build' and if, any errors try solving or you can share it here for discussion.
Adez017
left a comment
Adez017
left a comment
There was a problem hiding this comment.
reset of the things looks good.
|
also @mukeshdhadhariya , tag the issue number assigned to this . |
Synced data from Linked IssuesLabels:
Assignees:
Milestones:
|
npm run build -> successfull |
|
@Adez017 lmk if we good to close this issue |
Adez017
left a comment
Adez017
left a comment
There was a problem hiding this comment.
good to go from my side @sanjay-kv .
what are your thoughts @iitzIrFan
iitzIrFan
left a comment
iitzIrFan
left a comment
There was a problem hiding this comment.
Yes we an merge this, only remains is the documentation update, rest is fine @sanjay-kv @Adez017
Can you merge This issue? |
iitzIrFan
added
the
Ready for Merge
|
Im reviewing will merge it soon @mukeshdhadhariya |
let me know how the documentation is needed , will happy to work on that @sanjay-kv |
4 participants
Summary
constructororsetAuthToken()(server-side only). Update README with instantiation instructions:new GitHubService(process.env.GITHUB_TOKEN).If-None-Matchsupport and store ETags in cache meta. Conditional GET reduces API usage and avoids hitting rate limits.fetchWithRetrynow accepts custom headers and handles304 Not Modifiedresponses. Cache now stores{ data, etags, lastUpdated }.Why
Exposing tokens in client-side code risks leaking credentials and causing abuse. Using conditional requests via ETag reduces bandwidth and GitHub API rate consumption.
Developer notes
- Instantiate server-side with:
- Do not set tokens on
- Cache meta now includes ETags (keyed by endpoint) and
issue #891const svc = new GitHubService(process.env.GITHUB_TOKEN);windowin production.lastUpdatedfor freshness checks.