-
Notifications
You must be signed in to change notification settings - Fork 0
Update cache control settings for release manifests and downloads #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR refactors the caching strategy in the release proxy by removing middleware-based cache control and implementing explicit per-route Cache-Control headers. Routes now compute Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
espresense-release-proxy/src/index.ts (1)
80-82: Consider extracting the maxAge logic into a helper function.The maxAge calculation is duplicated in both the manifest and download endpoints. You could extract this into a reusable helper to improve maintainability:
🔎 Optional refactor suggestion
+function getCacheMaxAge(tag: string): number { + // latest changes frequently, specific releases are immutable + return tag === 'latest' ? 300 : 86400 +} + function esp32(path: string) {Then use it in both routes:
- // latest changes frequently, specific releases are immutable - const maxAge = tag === 'latest' ? 300 : 86400 + const maxAge = getCacheMaxAge(tag)Also applies to: 119-121
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
espresense-release-proxy/src/index.ts
🔇 Additional comments (3)
espresense-release-proxy/src/index.ts (3)
119-121: Same cache TTL mismatch as in the manifest endpoint.Similar to the manifest endpoint, the client-side
Cache-Controlallows specific release downloads to be cached for 1 day, while the Cloudflare edge cache (cacheTtlByStatuson line 125) revalidates every 5 minutes.Consider aligning the edge cache TTL with the client cache for specific releases if immutability is guaranteed.
Also applies to: 125-125
140-172: LGTM! Consistent caching for latest prerelease.The caching strategy for the latest prerelease endpoint is consistent, with both client-side and edge cache set to 5 minutes (300s). The explicit
Cache-Controlheader on the redirect response ensures proper cache behavior.
80-82: Confirm the cache strategy for specific releases between client and edge caches.The client-side
Cache-Controlheader allows specific releases to be cached for 1 day (86400s), but the Cloudflare edge cache (cacheTtlByStatuson lines 86, 125, and 148) is set to 300s for all responses. This means specific release manifests and downloads cache for 1 day on clients but revalidate on the edge every 5 minutes—inconsistent with the immutability stated in the code comments.If immutable releases should be cached longer on the edge, consider raising
cacheTtlByStatusto 86400 for non-latest tags. If the current setup is intentional (keeping the edge cache fresh), document the reasoning. Additionally, themaxAgecomputation is duplicated across endpoints (lines 81 and 120); consider extracting it to reduce duplication.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.