perf: add CDN Cache-Control headers to streak API route - #267
Conversation
|
@HarshParmar029 is attempting to deploy a commit to the jhasourav07's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
👋 Hey @HarshParmar029, welcome to CommitPulse! 🎉 Thanks for opening your first pull request — this is a big deal and we appreciate the effort! While you wait for a review, please double-check:
A maintainer will review your PR shortly. Hang tight! 🚀 |
|
👋 Hey @HarshParmar029, it looks like you didn't use our PR template! The section Please update your PR description to include all required sections so we can review this properly:
You can find the full template in CONTRIBUTING.md. Just edit your PR description and the |
|
🚨 Hey @HarshParmar029, the CI Pipeline is failing on this PR and it has been marked as Please fix the issues before this can be reviewed. Here's how: 1. Run checks locally before pushing: npm run format:check # Check Prettier formatting
npm run lint # Run ESLint
npm run typecheck # TypeScript type check
npm run test # Run unit tests (Vitest)
npm run build # Verify production build passes2. Auto-fix common issues: npm run format # Auto-fix formatting with Prettier
npm run lint -- --fix # Auto-fix lint errors where possible3. Check the full failure log here: Once you push a fix and the CI passes, the |
|
@HarshParmar029 follow PR #87 for further contributions in the repo |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
8 similar comments
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
1390e5b to
9eb5837
Compare
|
Deployment failed with the following error: |
6f906cb to
35f50c8
Compare
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
| run: npm run build | ||
| env: | ||
| CI: false | ||
| FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true |
There was a problem hiding this comment.
Why did you edited this ?
keep the PR focused to solve the issue please
35f50c8 to
1f66636
Compare
|
Fixed! Removed the extra FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 line from ci.yml and squashed all commits into one. PR is now focused only on CDN Cache-Control headers. |
|
Please pull the latest changes and resolve the conflicts so we can review it! git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseOnce resolved, the |
6bcadb0 to
a1ed434
Compare
884df70 to
2827302
Compare
|
"Hi @Aamod007, Fixed all issues: Thank you!" |
There was a problem hiding this comment.
Pull request overview
This PR is intended to implement CDN-friendly Cache-Control headers for the /api/streak route to reduce repeated serverless invocations and GitHub API usage (Fixes #69).
Changes:
- Removes/adjusts in-file comments and whitespace in
app/api/streak/route.ts. - (Intended, per PR description/issue #69) Update caching strategy for successful and error responses, but the current diff does not yet apply the described
s-maxage=14400behavior.
Comments suppressed due to low confidence (1)
app/api/streak/route.ts:103
- The Cache-Control logic here still uses a dynamic
s-maxage=${secondsToMidnight}(until midnight) rather than the fixeds-maxage=14400described in the PR and required by issue #69. As a result, the response header won’t match the expectedpublic, s-maxage=14400, stale-while-revalidate=86400value, and the related cache-control tests will also need updating to reflect the new fixed TTL.
const secondsToMidnight = tzParam
? getSecondsUntilMidnightInTimezone(timezone)
: getSecondsUntilUTCMidnight();
const cacheControl =
refresh || isRandomTheme
? 'no-cache, no-store, must-revalidate'
: `public, s-maxage=${secondsToMidnight}, stale-while-revalidate=86400`;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @Aamod007, Updated 500 error response to Cache-Control: public, s-maxage=60 as suggested by Copilot. All 205 tests passing. Could you please re-review? Thank you! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app/api/streak/route.ts:104
- The PR description/Issue #69 calls for a fixed CDN cache TTL (
s-maxage=14400) on successful responses, but the route still computessecondsToMidnightand setss-maxagedynamically based on UTC/local midnight. This doesn't match the described behavior (and the current tests are still written around the midnight-based TTL).
const secondsToMidnight = tzParam
? getSecondsUntilMidnightInTimezone(timezone)
: getSecondsUntilUTCMidnight();
const cacheControl =
refresh || isRandomTheme
? 'no-cache, no-store, must-revalidate'
: `public, s-maxage=${secondsToMidnight}, stale-while-revalidate=86400`;
|
🎉 Congratulations @HarshParmar029! Your PR has been successfully merged. 🚀 Thank you for contributing to CommitPulse. Your work helps us build a better tool for the community.
Keep building! 💻✨ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
app/api/streak/route.ts:104
- The success response Cache-Control is still using a dynamic
s-maxage=${secondsToMidnight}. The PR description/issue #69 call for a fixeds-maxage=14400(4 hours) withstale-while-revalidate=86400; as-is the route won’t match the intended CDN caching behavior (and requires the midnight helper calculations).
const secondsToMidnight = tzParam
? getSecondsUntilMidnightInTimezone(timezone)
: getSecondsUntilUTCMidnight();
const cacheControl =
refresh || isRandomTheme
? 'no-cache, no-store, must-revalidate'
: `public, s-maxage=${secondsToMidnight}, stale-while-revalidate=86400`;
| @@ -152,6 +142,7 @@ | |||
| }, | |||
| return new NextResponse(errorSvg, { | ||
| status: 500, | ||
| headers: { | ||
| 'Content-Type': 'image/svg+xml', | ||
| 'Cache-Control': 'no-cache', | ||
| 'Cache-Control': 'public, s-maxage=60', | ||
| }, | ||
| }); |
| it('never caches an error response', async () => { | ||
| vi.mocked(fetchGitHubContributions).mockRejectedValue(new Error('Network failure')); | ||
|
|
||
| const response = await GET(makeRequest({ user: 'octocat' })); | ||
|
|
||
| expect(response.headers.get('Cache-Control')).toBe('no-cache'); | ||
| expect(response.headers.get('Cache-Control')).toBe('public, s-maxage=60'); | ||
| }); |
) ## Description Fixes JhaSourav07#69 Added CDN Cache-Control headers to the streak API route: - Replaced dynamic s-maxage with fixed `s-maxage=14400` (4 hours) - Added `stale-while-revalidate=86400` (24 hours) - Updated error responses (500) to use `public, s-maxage=60` ## Pillar 🛠️ Other (Bug fix, refactoring, docs) ## Checklist - [x] I have read the CONTRIBUTING.md file. - [x] My commits follow the Conventional Commits format. - [x] I have made sure that I have only one commit to merge in this PR.
) ## Description Fixes JhaSourav07#69 Added CDN Cache-Control headers to the streak API route: - Replaced dynamic s-maxage with fixed `s-maxage=14400` (4 hours) - Added `stale-while-revalidate=86400` (24 hours) - Updated error responses (500) to use `public, s-maxage=60` ## Pillar 🛠️ Other (Bug fix, refactoring, docs) ## Checklist - [x] I have read the CONTRIBUTING.md file. - [x] My commits follow the Conventional Commits format. - [x] I have made sure that I have only one commit to merge in this PR.
Description
Fixes #69
Added CDN Cache-Control headers to the streak API route:
s-maxage=14400(4 hours)stale-while-revalidate=86400(24 hours)public, s-maxage=60Pillar
🛠️ Other (Bug fix, refactoring, docs)
Checklist