Add custom response header hook - #110
Open
vibhor-aggr wants to merge 2 commits into
Open
vibhor-aggr wants to merge 2 commits into
vibhor-aggr wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds an optional 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Reviewer's GuideAdds an optional setHeaders(ctx, file) hook to the staticCache middleware that runs after standard static headers are applied, along with tests and README documentation demonstrating custom headers for both streamed and buffered files. Sequence diagram for staticCache setHeaders hooksequenceDiagram
participant Client
participant Koa as KoaApp
participant staticCache
participant ctx
Client->>Koa: HTTP request
Koa->>staticCache: staticCache(ctx, next)
staticCache->>ctx: set(cache-control)
staticCache->>ctx: set(content-md5)
alt options.setHeaders is provided
staticCache->>staticCache: setHeaders(ctx, file)
end
ctx-->>Client: HTTP response with headers
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.js (1)
99-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
setHeadersis skipped for conditional 304 responses.For matched files with
If-None-Match/If-Modified-Since, the early return at Line 99 bypasses the hook, so custom security headers are never applied on fresh-cache hits.Suggested fix
- if (ctx.fresh) return ctx.status = 304 + if (ctx.fresh) { + if (setHeaders) setHeaders(ctx, file) + ctx.status = 304 + return + } ctx.type = file.type ctx.length = file.zipBuffer ? file.zipBuffer.length : file.length ctx.set('cache-control', file.cacheControl || 'public, max-age=' + file.maxAge) if (file.md5) ctx.set('content-md5', file.md5) if (setHeaders) setHeaders(ctx, file)🤖 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 `@index.js` around lines 99 - 105, The setHeaders callback is being invoked after the early return statement for fresh resources, causing it to be skipped when ctx.fresh is true and a 304 status is returned. Move the setHeaders invocation (the line with "if (setHeaders) setHeaders(ctx, file)") to execute before the fresh resource check at the beginning of this block so that custom headers are applied for both fresh-cache hits and regular responses.
🧹 Nitpick comments (1)
test/index.js (1)
253-304: ⚡ Quick winAdd a conditional-request regression test for
setHeaders.Current tests only validate 200 responses, so they won’t catch callback behavior on fresh-cache (
304) requests.Suggested test addition
+ it('should set custom headers for conditional requests', function (done) { + var app = new Koa() + app.use(staticCache(path.join(__dirname, '..'), { + setHeaders: function (ctx) { + ctx.set('X-Static-Conditional', '1') + }, + filter(file) { + return !file.includes('node_modules') + } + })) + + request(app.listen()) + .get('/index.js') + .expect(200) + .expect('X-Static-Conditional', '1') + .end(function (err, res) { + if (err) return done(err) + request(app.listen()) + .get('/index.js') + .set('If-None-Match', res.headers.etag) + .expect(304) + .expect('X-Static-Conditional', '1', done) + }) + })🤖 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 `@test/index.js` around lines 253 - 304, Add a new test case that validates the setHeaders callback behavior for conditional requests returning 304 Not Modified responses. Create a test similar to the existing streamed and buffered file header tests, but make two consecutive requests to the same file where the second request includes conditional headers (If-None-Match or If-Modified-Since) to trigger a 304 response. Verify that the setHeaders callback is still invoked during the 304 response and that the seenFile object is properly populated, ensuring the callback works correctly for both fresh (200) and cached (304) responses.
🤖 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.
Outside diff comments:
In `@index.js`:
- Around line 99-105: The setHeaders callback is being invoked after the early
return statement for fresh resources, causing it to be skipped when ctx.fresh is
true and a 304 status is returned. Move the setHeaders invocation (the line with
"if (setHeaders) setHeaders(ctx, file)") to execute before the fresh resource
check at the beginning of this block so that custom headers are applied for both
fresh-cache hits and regular responses.
---
Nitpick comments:
In `@test/index.js`:
- Around line 253-304: Add a new test case that validates the setHeaders
callback behavior for conditional requests returning 304 Not Modified responses.
Create a test similar to the existing streamed and buffered file header tests,
but make two consecutive requests to the same file where the second request
includes conditional headers (If-None-Match or If-Modified-Since) to trigger a
304 response. Verify that the setHeaders callback is still invoked during the
304 response and that the seenFile object is properly populated, ensuring the
callback works correctly for both fresh (200) and cached (304) responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b564def-5fe6-4650-853e-afb949eb21d4
📒 Files selected for processing (3)
README.mdindex.jstest/index.js
Author
|
Addressed the 304 path by running Local verification: focused custom-header Mocha tests passed, exact |
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
1 participant
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.
Closes #102.
This adds an optional
setHeaders(ctx, file)hook that runs after the built-in static headers have been applied for a matched file. It lets applications add or override custom response headers while preserving the existing buffered, streamed, cache, and gzip behavior.Verification:
./node_modules/.bin/mocha --grep "custom headers"npm testgit diff --checkSummary by Sourcery
Add a configurable hook to customize response headers for static file responses.
New Features:
Documentation:
Tests:
Summary by CodeRabbit
setHeaders(ctx, file)hook forstaticCache, enabling custom response headers per matched cached file.setHeadersoption and its parameters.setHeadersis invoked for both streamed and buffered responses, and that it also runs during conditional requests resulting in304.