fix: use precompiled gzip when streaming files#108
Conversation
|
Warning Review limit reached
More reviews will be available in 11 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends precompiled gzip support to streamed static responses. A new internal helper 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 support for serving precompiled .gz assets when responses are streamed (buffer: false), by detecting adjacent gzip files before creating dynamic gzip streams and by covering the behavior with a regression test. Sequence diagram for serving precompiled gzip when streamingsequenceDiagram
participant Client
participant KoaCtx as Koa_ctx
participant StaticCache as staticCache_middleware
participant FS as fs
Client->>KoaCtx: HTTP GET /asset
KoaCtx->>StaticCache: staticCache(dir, options, files)
StaticCache->>StaticCache: [shouldGzip]
StaticCache->>StaticCache: [options.usePrecompiledGzip]
StaticCache->>StaticCache: getPrecompiledGzip(file, filename, files)
alt [precompiledGzip found]
StaticCache->>KoaCtx: set content-encoding gzip
StaticCache->>KoaCtx: set length precompiledGzip.length
alt [precompiledGzip.buffer]
StaticCache->>KoaCtx: ctx.body = precompiledGzip.buffer
else [no buffer]
StaticCache->>FS: createReadStream(precompiledGzip.path)
FS-->>StaticCache: ReadableStream
StaticCache->>KoaCtx: ctx.body = ReadableStream
end
else [no precompiledGzip]
StaticCache->>FS: createReadStream(file.path)
FS-->>StaticCache: ReadableStream
StaticCache->>KoaCtx: ctx.body = ReadableStream (dynamic gzip or plain)
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The test that overrides
mzzlib.createGzipmutates a shared module-level singleton and only restores it in the.endcallback; consider using atry/finallyorafterEachhook to guarantee cleanup even if the test fails or throws earlier, to avoid leaking state into other tests. - In
getPrecompiledGzip,await fs.stat(gzPath)assumes a promise-basedfsAPI; iffsis Node’s core module here, this will never resolve as expected—either switch to the existing async wrapper you use elsewhere (e.g.mz/fs) or wrapfs.statin a promise in this function.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test that overrides `mzzlib.createGzip` mutates a shared module-level singleton and only restores it in the `.end` callback; consider using a `try/finally` or `afterEach` hook to guarantee cleanup even if the test fails or throws earlier, to avoid leaking state into other tests.
- In `getPrecompiledGzip`, `await fs.stat(gzPath)` assumes a promise-based `fs` API; if `fs` is Node’s core module here, this will never resolve as expected—either switch to the existing async wrapper you use elsewhere (e.g. `mz/fs`) or wrap `fs.stat` in a promise in this function.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
🧹 Nitpick comments (2)
test/index.js (1)
373-418: ⚡ Quick winConsider adding test coverage for edge cases.
The new test validates the happy path: precompiled gzip is correctly served in streaming mode when the
.gzfile exists. To strengthen confidence in the implementation, consider adding tests for:
- Missing
.gzfile: Verify that whenasset.js.gzdoes not exist, the middleware falls back to dynamically generating gzip (remove thecreateGzipmock or make it return a valid stream).- Cached precompiled gzip: Verify the code path where
files.get(filename + '.gz')returns a cached file object (line 172 ofindex.js).- Stale
.gzfile (if mtime validation is added per the previous comment): Verify behavior when the.gzfile is older than the original 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 `@test/index.js` around lines 373 - 418, Add tests covering the edge cases around serving precompiled gzips: (1) "Missing .gz file" — create a test similar to the existing one but do not write asset.js.gz and ensure the middleware falls back to dynamic gzip by restoring/allowing mzzlib.createGzip to work (reference mzzlib.createGzip and the staticCache invocation with gzip/usePrecompiledGzip options); (2) "Cached precompiled gzip" — simulate a cached entry by ensuring files.get(filename + '.gz') returns a file object (reference files.get and the filename + '.gz' lookup in index.js) and assert the middleware serves that cached object; (3) "Stale .gz file" — create asset.js and a gz file with an older mtime than asset.js and assert expected behavior (fallback or re-gzip per implementation). For each test, reuse the streaming/dynamic options (buffer:false, dynamic:true) and assert proper Content-Encoding/Content-Length and cleanup restores mzzlib.createGzip and removes tmp files.index.js (1)
139-148: ⚖️ Poor tradeoffETag does not reflect the gzipped representation.
The ETag (set at line 94 from
file.md5) represents the MD5 hash of the original uncompressed content. When serving the precompiled gzipped variant, the actual bytes differ, but the ETag remains unchanged. This extends the existing pattern from the buffered path (lines 122-137), where the same ETag is used for both compressed and uncompressed responses.While the
Vary: Accept-Encodingheader (line 82) instructs caches to store separate entries per encoding, not all HTTP caches handleVarycorrectly, and clients performing conditional requests may experience incorrect 304 responses if they switch between accepting and not accepting gzip.Per HTTP semantics, the ETag should uniquely identify each representation, including differences in
Content-Encoding. However, fixing this would require computing separate ETags for gzipped variants and updating cache invalidation logic across both buffered and streaming paths.🤖 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 139 - 148, The ETag currently uses file.md5 (original uncompressed content) even when serving precompiledGzip; compute and set a distinct ETag for the gzipped representation before returning so responses encoded with gzip have a unique validator. In the precompiledGzip branch (where precompiledGzip is returned from getPrecompiledGzip), derive an MD5 (or other strong hash) from the gzipped bytes (use precompiledGzip.buffer if present, or stream/read precompiledGzip.path to compute the hash), then call ctx.set('etag', computedGzipEtag) (instead of leaving file.md5) and keep the Vary header; ensure the same approach is applied consistently with the buffered compressed path that uses file.md5 so each Content-Encoding variant has its own ETag.
🤖 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 `@index.js`:
- Around line 171-189: In getPrecompiledGzip, after obtaining stats for the .gz
file (gzPath), also stat the original file (file.path) and compare modification
times; if the .gz mtime is older than the original file's mtime return null so
stale precompressed files are not used. Specifically, inside getPrecompiledGzip,
call fs.stat on file.path (e.g., origStats), compare gzStats.mtimeMs (or mtime)
with origStats.mtimeMs, and only return the { path: gzPath, length: stats.size }
when gz is equal or newer; otherwise return null. Ensure errors from
stat(file.path) propagate similarly to the current error handling for gzPath.
---
Nitpick comments:
In `@index.js`:
- Around line 139-148: The ETag currently uses file.md5 (original uncompressed
content) even when serving precompiledGzip; compute and set a distinct ETag for
the gzipped representation before returning so responses encoded with gzip have
a unique validator. In the precompiledGzip branch (where precompiledGzip is
returned from getPrecompiledGzip), derive an MD5 (or other strong hash) from the
gzipped bytes (use precompiledGzip.buffer if present, or stream/read
precompiledGzip.path to compute the hash), then call ctx.set('etag',
computedGzipEtag) (instead of leaving file.md5) and keep the Vary header; ensure
the same approach is applied consistently with the buffered compressed path that
uses file.md5 so each Content-Encoding variant has its own ETag.
In `@test/index.js`:
- Around line 373-418: Add tests covering the edge cases around serving
precompiled gzips: (1) "Missing .gz file" — create a test similar to the
existing one but do not write asset.js.gz and ensure the middleware falls back
to dynamic gzip by restoring/allowing mzzlib.createGzip to work (reference
mzzlib.createGzip and the staticCache invocation with gzip/usePrecompiledGzip
options); (2) "Cached precompiled gzip" — simulate a cached entry by ensuring
files.get(filename + '.gz') returns a file object (reference files.get and the
filename + '.gz' lookup in index.js) and assert the middleware serves that
cached object; (3) "Stale .gz file" — create asset.js and a gz file with an
older mtime than asset.js and assert expected behavior (fallback or re-gzip per
implementation). For each test, reuse the streaming/dynamic options
(buffer:false, dynamic:true) and assert proper Content-Encoding/Content-Length
and cleanup restores mzzlib.createGzip and removes tmp files.
🪄 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 Plus
Run ID: 41cfa9d8-a5f5-4168-8456-320342f63ae0
📒 Files selected for processing (2)
index.jstest/index.js
1 participant
Closes #100.
usePrecompiledGzippreviously only used the cached.gzfile when the original response was buffered. Withbuffer: false, the middleware streamed the original file and dynamically piped it through gzip even when an adjacent precompiled.gzfile existed.This keeps the existing gzip eligibility checks, preserves buffered behavior, and adds a streaming path for the precompiled gzip file.
Verification:
./node_modules/.bin/mocha --grep 'precompiled gzip when streaming dynamic files'npm testgit diff --checkSummary by Sourcery
Serve precompiled gzip assets when streaming static files with gzip enabled.
New Features:
Bug Fixes:
Tests:
Summary by CodeRabbit
New Features
Tests