Fix: Headers not forwarded to clients, incorrect Content-Type#31
Open
KevinPayravi wants to merge 3 commits into
Open
Fix: Headers not forwarded to clients, incorrect Content-Type#31KevinPayravi wants to merge 3 commits into
KevinPayravi wants to merge 3 commits into
Conversation
All thumbnails in S3 are stored as .jpg, but may not be stored with the correct content type, causing S3 to return application/octet-stream as the default value. This overrides the Content-Type to ensure clients always receive type image/jpeg.
WalkthroughResponseHelper.getHeadersFromTarget is refactored to return a plain object instead of a Map, selecting only ChangesHeader Type Refactoring
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ResponseHelper.ts (1)
98-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange
getCacheHeadersreturn type fromMap<string, string>toRecord<string, string>.
getCacheHeadersis passed toexpressResponse.set()at lines 80 and 144 insrc/ThumbnailApi.ts. Express'sres.set()expects a plain object and usesObject.keys()/ enumerable properties internally, which doesn't work with Maps. This matches the pattern already established bygetHeadersFromTarget, which returnsRecord<string, string>for the same purpose.🔧 Proposed fix
-getCacheHeaders(seconds: number): Map<string, string> { +getCacheHeaders(seconds: number): Record<string, string> { const now = Date.now(); const expirationDateString = new Date(now + 1000 * seconds).toUTCString(); const cacheControl = `public, max-age=${String(seconds)}`; - return new Map([ - ["Cache-Control", cacheControl], - ["Expires", expirationDateString], - ]); + return { + "Cache-Control": cacheControl, + "Expires": expirationDateString, + }; }And update the test in
test/unit/ResponseHelper.test.ts:test("getCacheHeaders", () => { const result = responseHelper.getCacheHeaders(123); - expect(result.get("Cache-Control")).toBe("public, max-age=123"); - const expires = result.get("Expires"); + expect(result["Cache-Control"]).toBe("public, max-age=123"); + const expires = result["Expires"]; expect(expires).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ResponseHelper.ts` around lines 98 - 106, The getCacheHeaders function currently returns a Map which is incompatible with expressResponse.set (used from ThumbnailApi at the places where getCacheHeaders is passed); change getCacheHeaders's return type from Map<string, string> to Record<string, string>, build and return a plain object with the same keys ("Cache-Control" and "Expires") instead of a Map, and update the corresponding unit test in test/unit/ResponseHelper.test.ts to expect a Record<string,string> (plain object) instead of a Map; ensure callers that pass the result to expressResponse.set still work without changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ThumbnailApi.ts`:
- Around line 159-165: Add a unit test that verifies
ThumbnailApi.serveItemFromS3 overrides Content-Type to image/jpeg: mock
ResponseHelper.getRemoteImagePromise to return a Response with headers
containing "application/octet-stream", mock ResponseHelper.getHeadersFromTarget
to return {"Content-Type": "application/octet-stream"}, and mock
ResponseHelper.pipe; construct ThumbnailApi with the mocked ResponseHelper and
call serveItemFromS3 with a fake express response, then assert that the express
response.set (or setHeaders) was called with an object containing
"Content-Type": "image/jpeg" (reference ThumbnailApi.serveItemFromS3,
ResponseHelper.getRemoteImagePromise, ResponseHelper.getHeadersFromTarget, and
the express response.set call).
---
Outside diff comments:
In `@src/ResponseHelper.ts`:
- Around line 98-106: The getCacheHeaders function currently returns a Map which
is incompatible with expressResponse.set (used from ThumbnailApi at the places
where getCacheHeaders is passed); change getCacheHeaders's return type from
Map<string, string> to Record<string, string>, build and return a plain object
with the same keys ("Cache-Control" and "Expires") instead of a Map, and update
the corresponding unit test in test/unit/ResponseHelper.test.ts to expect a
Record<string,string> (plain object) instead of a Map; ensure callers that pass
the result to expressResponse.set still work without changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b37000c-91b4-44ee-ac99-23fbe0d771ee
📒 Files selected for processing (4)
src/ResponseHelper.tssrc/ThumbnailApi.tstest/unit/ResponseHelper.test.tstest/unit/ThumbnailApi.test.ts
Comment on lines
+159
to
+165
| const headers = this.responseHelper.getHeadersFromTarget(response.headers); | ||
| // All thumbnails in S3 are stored as .jpg, | ||
| // but may not be stored with the correct content type, | ||
| // causing S3 to return application/octet-stream as the default value. | ||
| // This overrides the Content-Type to ensure clients always receive type image/jpeg. | ||
| headers["Content-Type"] = "image/jpeg"; | ||
| expressResponse.set(headers); |
There was a problem hiding this comment.
Add test coverage for the Content-Type override.
The Content-Type override is a key fix in this PR but lacks explicit test verification. The existing test at line 142 ("serveItemFromS3: success") only verifies that set is called, but doesn't confirm that Content-Type: image/jpeg is actually set regardless of the S3 response headers.
📋 Suggested test
Add a test case in test/unit/ThumbnailApi.test.ts that mocks S3 returning application/octet-stream and verifies the override:
test("serveItemFromS3: overrides Content-Type to image/jpeg", async () => {
const responseHelper = new ResponseHelper();
responseHelper.getRemoteImagePromise = jest.fn(() =>
Promise.resolve({
status: 200,
body: new ReadableStream(),
headers: new Headers([["content-type", "application/octet-stream"]]),
} as Response),
);
// Mock should return what S3 actually sends
responseHelper.getHeadersFromTarget = jest.fn(() => ({
"Content-Type": "application/octet-stream"
}));
responseHelper.pipe = jest.fn();
const thumbnailApi = new ThumbnailApi(
dplaApi as unknown as DplaApi,
thumbnailStorage,
thumbnailCacheQueue,
responseHelper,
);
const mockResponse = new ExpressResponse();
await thumbnailApi.serveItemFromS3(
itemId,
mockResponse as unknown as express.Response,
);
// Verify that the override happened
expect(mockResponse.set).toHaveBeenCalledWith(
expect.objectContaining({ "Content-Type": "image/jpeg" })
);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ThumbnailApi.ts` around lines 159 - 165, Add a unit test that verifies
ThumbnailApi.serveItemFromS3 overrides Content-Type to image/jpeg: mock
ResponseHelper.getRemoteImagePromise to return a Response with headers
containing "application/octet-stream", mock ResponseHelper.getHeadersFromTarget
to return {"Content-Type": "application/octet-stream"}, and mock
ResponseHelper.pipe; construct ThumbnailApi with the mocked ResponseHelper and
call serveItemFromS3 with a fake express response, then assert that the express
response.set (or setHeaders) was called with an object containing
"Content-Type": "image/jpeg" (reference ThumbnailApi.serveItemFromS3,
ResponseHelper.getRemoteImagePromise, ResponseHelper.getHeadersFromTarget, and
the express response.set call).
DominicBM
requested changes
May 11, 2026
Contributor
DominicBM
left a comment
DominicBM
left a comment
There was a problem hiding this comment.
I am not familiar enough with the code to give a detailed review, but it looks worthwhile. My Claude flags CodeReview's getCacheHeaders suggestion above as useful.
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
2 participants
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.
This PR resolves two issues related to incorrect thumbnail headers.
Header Forwarding
When opening a thumbnail (
dp.la/thumb/...), I noticed that the browser prompts me to download the file instead of displaying it. Looking closer, I noticed the thumbnails are missing aContent-Typeheader (which is one of the two headers we intend to send alongsideLast-Modified).ResponseHelper.getHeadersFromTargetwas returning aMap<string, string>, but Express'sres.set()requires a plain object. Passing a Map causedObject.keys()to return an empty array, so no headers were ever set on the response. So our intendedContent-TypeandLast-Modifiedheaders would be dropped.This is fixed by changing
getHeadersFromTargetto returnRecord<string, string>instead ofMap<string, string>. I've also updated the corresponding tests, which masked the issue because they called.get()directly on the returned Map (which works), and the ThumbnailApi tests used a jest-express mock forres.set()that doesn't enforce Express's plain-object requirement.Incorrect Content-Type
With the above fix, our thumbnail API started providing
Content-Type, but it wasapplication/octet-streaminstead ofimage/jpeg. The issue is that our thumbnails are stored in S3 withoutContentTypeset, so they default toapplication/octet-stream. I've opened a PR in our thumbnailer lambda to fix this: dpla/thumbnailer-lambda#12As a stopgap, this PR overrides the
Content-Typefrom S3 with valueimage/jpeg.Testing
Thumbnail headers before:
Thumbnail headers after:
Overview
This PR fixes two header-related issues in the thumbnail API:
Headers not being forwarded to clients:
ResponseHelper.getHeadersFromTarget()was returning aMap<string, string>, but Express'sres.set()requires a plain object. When passed a Map, Object.keys() returned an empty array, causing no headers to be set. The method now returnsRecord<string, string>instead.Incorrect Content-Type for S3 thumbnails: Thumbnails stored in S3 lack ContentType metadata and S3 returns
application/octet-stream. As a stopgap (pending a fix in the thumbnailer lambda), the API now overrides the S3-derived Content-Type toimage/jpegbefore responding to clients.Changes
getHeadersFromTarget()to return a plain object instead of a Map, with logic refactored to build the object by copying onlyContent-TypeandLast-Modifiedheaders when present.serveItemFromS3()now explicitly sets Content-Type toimage/jpegbefore forwarding headers to the client.API Response Changes
This PR changes the public-facing API response by fixing header forwarding—responses will now include
Content-Type: image/jpegandLast-Modifiedheaders that were previously not being sent to clients due to the Map/plain-object incompatibility bug.Notes