Repository navigation
Let browsers cache GetImage responses - #78
Merged
Merged
Conversation
Image IDs are random and never reused, but GetImage sent no cache headers, so every page view re-fetched each image through the Worker and the GitHub API (~1-2s). Found images are now immutable for a year with an ID-based ETag (If-None-Match gets a 304 without calling GitHub), and the body is streamed instead of buffered into a Blob. GitHub errors used to come back as 200 image/png with the error JSON as the body; they now keep GitHub's status and are marked no-store, and an unreachable GitHub returns 502, so a transient failure is never cached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's GuideUpdates GetImage to stream GitHub image responses with immutable, ID-based browser caching and ETag revalidation, while ensuring GitHub and network failures retain appropriate error statuses and are never cached; tests cover all new response paths. Sequence diagram for cached GetImage responsessequenceDiagram
participant Browser
participant Worker
participant GitHub
Browser->>Worker: GET GetImage?ImageID
alt If-None-Match matches ImageID
Worker-->>Browser: 304 with immutable cache headers
else Cache miss
Worker->>GitHub: GET image content
GitHub-->>Worker: 200 image stream
Worker-->>Browser: 200 streamed image with ETag
end
Sequence diagram for uncached GetImage failuressequenceDiagram
participant Browser
participant Worker
participant GitHub
Browser->>Worker: GET GetImage?ImageID
Worker->>GitHub: GET image content
alt GitHub returns an error
GitHub-->>Worker: Error status and JSON body
Worker-->>Browser: Same error status with no-store
else GitHub is unreachable
GitHub-->>Worker: Network failure
Worker-->>Browser: 502 with no-store
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Author
|
I should add this is why our service feels slower than @langningchen 's https://image.langningchen.com/ |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Reject any ImageID that is not 32 lowercase letters (the only format UploadImage issues; every stored image matches) with 400 before building the PAT-authenticated GitHub URL, where "../" segments resolved to other paths in the repo. Match If-None-Match with weak comparison over a tag list, per RFC 9110, so W/"id" or "a", "id" still get a 304. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Adds browser caching (1-year immutable, ETag/304) to GetImage, fixes GitHub error and 502 status passthrough, and validates ImageIDs, with tests. Needs human sign-off on the 1-year immutable cache tradeoff for the public endpoint.
View guided diff | Turn on auto-fix | Re-trigger cubic
Admins embed the project logo in discussion-board posts as GetImage?ImageID=logo / logo.png, which the 32-letter ID check rejected. Allow those two IDs, but cache them for a day without the ID-based ETag: unlike random upload IDs, the logo can be replaced in place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR aim to accomplish?:
Images served by
GetImage(e.g.https://assets.xmoj-script.uk/GetImage?ImageID=…) load slowly on every page view: the response has noCache-Control,ETagorLast-Modified, so browsers can't cache it, and each view goes Worker → GitHub API again (measured ~0.7–2s per view; a conditional request still returned the full 200).It also fixes a latent bug: when GitHub fails (404, rate limit),
GetImagereturned 200image/pngwith GitHub's error JSON as the body.How does this PR accomplish the above?:
Image IDs are 32 random letters and never reused, so a found image is safe to cache forever.
Cache-Control: public, max-age=31536000, immutableandETag: "<ImageID>"; stillcontent-type: image/png.If-None-Matchmatching the ID returns304without calling GitHub.Blob.no-store, and an unreachable GitHub returns502withno-store, so a transient failure is never pinned in browser caches.?1=1query on the GitHub API URL: authenticated API responses aren't cached by the Worker'sfetch, so it had no effect.Tests: 4 new cases in
test/process.test.js(immutable + bytes, 304 without fetch, error passthrough uncached, 502 uncached), all throughProcess().npm test: 152/152 pass;wrangler deploy --dry-runbuilds.Does this PR need a documentation update?:
No.
By submitting this pull request, I confirm the following:
devfor XMOJ-Script,masterfor everything else).git rebase)/webpages)/webpages)🤖 Generated with Claude Code
Summary by Sourcery
Improve
GetImagecaching, validation, and error handling so image delivery is faster and failures are not incorrectly cached.New Features:
GetImage.GetImagewith replaceable, short-lived caching.Bug Fixes:
Enhancements:
Tests:
Summary by cubic
Lets browsers cache
GetImageresponses so images no longer take roughly 1–2 seconds to load on every page view.Found images are now marked
cache-control: public, max-age=31536000, immutablewith an ID-basedETag; a matchingIf-None-Matchreturns304without calling GitHub, and the image body is streamed instead of buffered into aBlob. Matching uses weak comparison over a tag list per RFC 9110, soW/"id"or"a", "id"still revalidate.Also fixes a bug where GitHub errors were returned as
200 image/pngwith the error JSON as the body. Error responses now keep GitHub's status, and an unreachable GitHub returns502; both are markedno-storeso a transient failure is never pinned in browser caches. Image IDs that aren't 32 lowercase letters (the only formatUploadImageissues) are rejected with400before reaching GitHub, where../segments could resolve to other repo paths. The project logo remains reachable vialogo/logo.png, cached for a day without an ID-basedETagso it can be replaced in place.Written for commit 3cd7fc9. Summary will update on new commits.