fix: return content-type headers on non-bun runtimes#79
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors the static file-serving plugin to fix missing ChangesStatic File Serving Refactor with Caching & ETag Support
Sequence DiagramsequenceDiagram
actor Client
participant Server as Elysia Server<br/>(staticPlugin)
participant FileCache as LRUCache<br/>(CachedFile)
participant FileSystem as Filesystem
participant ETag as ETag<br/>Generator
Client->>Server: GET /public/style.css
Server->>FileCache: Check if cached?
alt Cache Hit
FileCache-->>Server: CachedFile{Blob, Stats, etag}
Note over Server: Extract etag from cache
Server->>Server: Check If-None-Match header
alt ETag Match (via alreadyCachedDownstream)
Server-->>Client: 304 Not Modified
else ETag Mismatch
Server-->>Client: 200 + Content-Type + etag + cache-control
end
else Cache Miss
FileCache-->>Server: null
Server->>FileSystem: getFile(path)
FileSystem-->>Server: ElysiaFile{path, value}
Server->>FileSystem: getFileStats(path)
FileSystem-->>Server: Stats{mtime, ...}
Server->>ETag: generateETag(ElysiaFile)
ETag-->>Server: etag string
Server->>FileCache: Store CachedFile{Blob, Stats, etag}
Server->>Server: Build response headers<br/>(Content-Type, etag, cache-control)
Server-->>Client: 200 + headers + body
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Add coverage for cached static file responses to ensure ETag, Cache-Control, 304 revalidation, custom headers, content-type, and index.html behavior remain consistent after an ElysiaFile is served from the LRU cache. Update the cached response path so cached files run through the same header and conditional request logic as uncached files.
Contributor
|
Note: both in the base and the edited version with more tests, it fails the |
Contributor
Author
|
you might need to run |
yuriiShmal
force-pushed
the
fix/content-type-headers
branch
from
17ef23f to
b41bed8
Compare
Contributor
Yep, but the issue now is twofold.
Test OutputEdit: Issues resolved! |
Replace the unresolved elysia/cookies subpath import with the main Elysia Context type for the handler set object.
Contributor
|
Note: there was an issue with |
Ensure dist artifacts are generated before the test suite runs, since the Node CJS/ESM tests import the package through its compiled exports.
Contributor
Author
? |
Contributor
Author
|
@JoshuaNam do u want to add a CI check for |
Contributor
`bun run build` output before changes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 60: The package.json contains a "dev:node" npm script that invokes "tsx"
but "tsx" is not declared in devDependencies; either add "tsx" to
devDependencies (so the script can run in CI and fresh checkouts) or update the
"dev:node" script to use "bunx tsx" if you prefer Bun. Locate the "dev:node"
script in package.json and either add a devDependency entry for "tsx" with an
appropriate version, or change the script value from "tsx watch
example/index.ts" to "bunx tsx watch example/index.ts".
In `@src/index.ts`:
- Around line 92-113: finalizeFileResponse always recomputes the ETag by calling
generateETag(file), causing fs.readFile and hashing on every cache hit; modify
finalizeFileResponse to first check for a cached etag on the ElysiaFile instance
(e.g., file.etag) and only call generateETag(file) if that property is missing,
and when you compute an etag store it on file.etag so subsequent requests reuse
it; apply the same pattern to the other call site referenced (lines ~117-119) so
both paths avoid repeated disk reads and hashing.
- Around line 173-182: The current guard "if (files.length <= staticLimit) { ...
}" causes the static-file mounting loop in src/index.ts to be skipped entirely
when a large asset directory exceeds staticLimit, which prevents adding the
catch-all static route when alwaysStatic is true and skips bun HTML pre-bundling
(shouldBundleFileWithBun) for isBun && bunFullstack; update the logic so that
large directories still register the necessary catch-all/static route when
alwaysStatic is true and still handle pre-bundling of .html files when isBun &&
bunFullstack (e.g., remove or narrow the files.length <= staticLimit outer guard
and instead apply staticLimit per-file where appropriate, or add a separate code
path that ensures the catch-all route and .html pre-bundling run regardless of
files.length), referencing the loop variables files, staticLimit, alwaysStatic,
isBun, bunFullstack, and shouldBundleFileWithBun.
- Around line 197-203: The current loop returns new Elysia() when
fileExists(absoluteFilePath) is false, which discards all previously registered
routes; instead, change that return into a skip so the plugin continues
processing remaining files: when fileExists(absoluteFilePath) is false, log the
warning (if !silent) and continue the loop rather than returning a new Elysia()
(so remove/replace the return new Elysia() in the same block), ensuring the rest
of listFiles() processing and previously registered routes remain intact.
- Around line 252-260: The catch-all uses params['*'] joined to assets allowing
../ path traversal; fix getFileResponse invocation by resolving and validating
the target path before calling normalizePath: decode the param (fastDecodeURI or
raw), construct a candidate via path.resolve(assets, decoded), then ensure the
resolved path is inside the assets root (e.g., check that resolvedPath ===
assetsResolved or resolvedPath.startsWith(assetsResolved + path.sep) or use
path.relative to ensure it doesn't start with '..'); if validation fails, return
a 400/403 or a safe error instead of reading the file. Use the symbols assets,
params['*'], fastDecodeURI, path.resolve/path.relative, normalizePath and
getFileResponse in the changes.
In `@src/utils.ts`:
- Around line 204-217: generateETag currently rereads and hashes the file on
every call (in generateETag), negating file caching; fix by introducing an etag
cache (reuse existing LRUCache or a Map) keyed by
`${file.path}:${stat.mtimeMs}:${stat.size}` and return cached value when
present, otherwise compute the etag once (using file.value when available or
reading the file only on first compute), store it in the cache, and return it;
ensure cache key uses the file's mtimeMs and size so the entry auto-invalidates
when the file changes and update references to Crypto/Bun hashing logic in
generateETag to consult and populate this cache.
In `@test/index.test.ts`:
- Around line 536-554: The test "returns 304 for if-none-match after the file
has been cached" currently only checks status and body, which can miss
regressions where cache validators aren't preserved; update the assertion block
after computing second to also assert that second.headers.get('etag') strictly
equals the previously saved etag and that second.headers.get('cache-control')
(or the expected cache header name) is present and matches the expected value,
keeping the existing status/body checks; apply the same hardening to the other
similar tests (the ones asserting 304 at the later ranges) so each 304 response
verifies preserved ETag and Cache-Control headers using the same etag variable
and header checks referenced in this test.
🪄 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: 61e970ea-b3b2-4282-b81c-ff84368b5505
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.gitignoreexample/index.tspackage.jsonpublic/js/index.jssrc/index.tssrc/types.tssrc/utils.tstest/index.test.tstest/utils.ts
…cause wildcard doesn't catch that path
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
bench/index.ts (2)
28-35: 💤 Low valueLeftover commented-out debug lines everywhere — did you just forget to clean up? How embarrassing~ (≖‿≖)
Lines 28, 30, and 35 all have commented-out
console.logcalls. They add noise and signal that this file is still in a "debugging" state rather than a committed benchmark harness♡✨ Proposed fix
for (const path of htmlPaths) { - // console.log(path) const res = await app.handle(req(path)) - // await (await res.blob()).text() + await res.blob() } await bench.run() -// console.log(bench.name) console.table(bench.table())🤖 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 `@bench/index.ts` around lines 28 - 35, This file contains leftover commented-out debug statements (commented console.log calls) around the benchmark run—remove the noisy comments to clean up the committed code: delete the commented lines near the call to app.handle(req) (the commented "console.log(path)"), the commented await of res.blob().text(), and the trailing commented "console.log(bench.name)" so bench.run(), app.handle(req), req, and res remain as the only active code paths and the benchmark harness is tidy.
18-18: 💤 Low valueDebug
console.logleft in production benchmark output, how sloppy~ (￣ω￣;)
console.log(app.routes)at line 18 dumps the full route table every time the benchmark runs, polluting the output. Not a great look for a harness that usesconsole.tablefor clean results~♡✨ Proposed fix
-console.log(app.routes)🤖 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 `@bench/index.ts` at line 18, Remove the stray debug print by deleting or gating the console.log(app.routes) call so the route table is not printed during normal benchmark runs; either remove the line entirely or wrap it in a debug-only guard (e.g., check process.env.DEBUG or a --debug flag) before calling console.log(app.routes) so production runs and console.table output remain clean.
🤖 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 `@bench/index.ts`:
- Around line 29-31: The benchmark obtains a Response from app.handle(req(path))
but never consumes the response body (res is unused), so the benchmark skips
I/O/serialization cost; fix by draining the response inside the loop — e.g.,
await the body promise returned by res.blob() or res.text() (uncomment and await
the existing line), or use await res.arrayBuffer() for binary work — so the
route caching benchmark measures a full round-trip; update the code around the
app.handle(req(path)) call and the res variable to ensure the body is awaited
and any errors are propagated.
- Line 20: The benchmark callback currently awaits app.modules on every
iteration (await app.modules), which skews timing; move the await out of the
per-iteration callback by resolving app.modules once before registering/running
the benchmark (e.g., const modules = await app.modules) and then use the
resolved modules variable inside the benchmark function instead of awaiting
app.modules each iteration; update references in the benchmark callback to use
the resolved value so only the actual work is measured.
In `@bun-build/index.css`:
- Line 75: The font-family declaration in the `@font-face` block currently uses
quoted "Inter", which violates the stylelint font-family-name-quotes rule; fix
it by removing the quotes so the declaration reads font-family: Inter; (locate
the `@font-face` rule containing the font-family property and replace the quoted
"Inter" token with the unquoted Inter identifier).
In `@test/utils.d.ts`:
- Line 2: Replace the non-portable type by changing the exported declaration for
takodachi: currently "export declare const takodachi: string | NonSharedBuffer;"
— remove NonSharedBuffer and use a portable binary type such as Uint8Array so it
reads "string | Uint8Array"; update any related type usages of takodachi to
accept Uint8Array where applicable (references: the takodachi export in the
declaration file).
---
Nitpick comments:
In `@bench/index.ts`:
- Around line 28-35: This file contains leftover commented-out debug statements
(commented console.log calls) around the benchmark run—remove the noisy comments
to clean up the committed code: delete the commented lines near the call to
app.handle(req) (the commented "console.log(path)"), the commented await of
res.blob().text(), and the trailing commented "console.log(bench.name)" so
bench.run(), app.handle(req), req, and res remain as the only active code paths
and the benchmark harness is tidy.
- Line 18: Remove the stray debug print by deleting or gating the
console.log(app.routes) call so the route table is not printed during normal
benchmark runs; either remove the line entirely or wrap it in a debug-only guard
(e.g., check process.env.DEBUG or a --debug flag) before calling
console.log(app.routes) so production runs and console.table output remain
clean.
🪄 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: 9210c505-e44b-47c3-8c5b-6cd62a65114d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitignorebench/index.d.tsbench/index.tsbun-build/index.cssbun-build/index.htmlbun-build/src/client/frontend.d.tsbun-build/src/client/frontend.tsxexample/index.tspackage.jsonpublic/html/index.htmlsrc/index.tssrc/utils.tstest/index.test.tstest/utils.d.tstsconfig.dts.jsontsconfig.json
✅ Files skipped from review due to trivial changes (5)
- bench/index.d.ts
- public/html/index.html
- tsconfig.dts.json
- .gitignore
- bun-build/src/client/frontend.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- example/index.ts
- package.json
- src/index.ts
- test/index.test.ts
Co-authored-by: tiago <70700766+tiagozip@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/index.test.ts (1)
650-668:⚠️ Potential issue | 🟠 Major | ⚡ Quick win304 assertions are still too soft, you missed the sneaky part~ (¬‿¬)♡
These cases only assert
304+ empty body. They should also assert preservedetagandcache-controlon the 304 response, otherwise header regressions can slip through.Suggested hardening pattern (apply to all 3 tests)
const second = await app.handle(request) expect(second.status).toBe(304) expect(second.body).toBe(null) + expect(second.headers.get('etag')).toBe(etag) + expect(second.headers.get('cache-control')).toBeTruthy()Also applies to: 693-716, 740-758
🤖 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.test.ts` around lines 650 - 668, The 304 tests only check status and empty body but must also assert response headers are preserved; update the test "returns 304 for if-none-match after the file has been cached" (and the similar tests around the other occurrences) to additionally assert that second.headers.get('etag') equals the original etag and that second.headers.get('cache-control') matches the cache-control from the first response (or the expected value from staticPlugin()), ensuring the ETag and Cache-Control headers are present and unchanged on the 304 response; locate the assertions around variables first, etag, request, and second to add these header checks.
🤖 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 `@test/index.test.ts`:
- Around line 151-156: The test "always static with assets on an absolute path"
passes a relative string ('public/') to staticPlugin.assets, so it doesn't
exercise the absolute-path bug; update the test to supply a real absolute path
(e.g., use Node's path.resolve or path.join with __dirname or process.cwd())
when calling staticPlugin({ alwaysStatic: true, assets: ... }) and add the
necessary path import/require in the test file so the assets argument is
genuinely absolute.
---
Duplicate comments:
In `@test/index.test.ts`:
- Around line 650-668: The 304 tests only check status and empty body but must
also assert response headers are preserved; update the test "returns 304 for
if-none-match after the file has been cached" (and the similar tests around the
other occurrences) to additionally assert that second.headers.get('etag') equals
the original etag and that second.headers.get('cache-control') matches the
cache-control from the first response (or the expected value from
staticPlugin()), ensuring the ETag and Cache-Control headers are present and
unchanged on the 304 response; locate the assertions around variables first,
etag, request, and second to add these header checks.
🪄 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: af6d11cc-767f-4f5d-8f33-3bea199d8c5b
📒 Files selected for processing (4)
bench/index.tsexample/index.tssrc/index.tstest/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- bench/index.ts
- example/index.ts
- src/index.ts
cirex-web
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
example/index.ts (1)
5-21: ⚡ Quick winAdd explicit startup error handling, baka~ (￣▽￣)♡
If
staticPlugin(...),.listen(...), or module init rejects, this path can fail without a controlled exit/log flow. Add a.catchon the IIFE so startup failures are deterministic.Suggested patch
-;(async () => { +;(async () => { const app = new Elysia(isBun ? {} : { adapter: node() }) .use( await staticPlugin({ prefix: 'hi', assets: 'public', alwaysStatic: true, bunFullstack: true, decodeURI: false, etag: false // staticLimit: 1 }) ) .listen(3005) await app.modules console.log(app.routes) -})() // no top-level awaits allowed for cjs (error triggered by `bun dev:node`) (idk how to fix this) +})().catch((error) => { + console.error('Failed to start example server:', error) + process.exitCode = 1 +})🤖 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 `@example/index.ts` around lines 5 - 21, The top-level async IIFE that constructs Elysia, calls staticPlugin(...), .listen(3005) and awaits app.modules can reject without handling; update the invocation of the IIFE (the anonymous async function wrapping Elysia/staticPlugin/listen/await app.modules) to attach a .catch handler that logs the error (including the error object) and exits the process (or otherwise returns a non-zero status) so startup failures from staticPlugin, .listen, or app.modules are reported and cause a deterministic shutdown.
🤖 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.
Nitpick comments:
In `@example/index.ts`:
- Around line 5-21: The top-level async IIFE that constructs Elysia, calls
staticPlugin(...), .listen(3005) and awaits app.modules can reject without
handling; update the invocation of the IIFE (the anonymous async function
wrapping Elysia/staticPlugin/listen/await app.modules) to attach a .catch
handler that logs the error (including the error object) and exits the process
(or otherwise returns a non-zero status) so startup failures from staticPlugin,
.listen, or app.modules are reported and cause a deterministic shutdown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a08ea39-53da-4042-a1f8-86bb987cd557
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpublic/kyuukurarin.mp4is excluded by!**/*.mp4
📒 Files selected for processing (4)
example/index.tspackage.jsonsrc/index.tstest/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/index.ts
- test/index.test.ts
Contributor
Author
|
Video streaming is still a bit jank on Node (waits for the entire file to load locally before playing from the start) and sometimes crashes the entire server if you reload the page enough times... (Without content-type headers, as it was previously, the video would just trigger a browser download, so I think this is a marginal improvement.) I think the issue is probably upstream since the only thing I'm returning is an entire blob chunk. I also tried alternative ways of returning the video as follows: |
Contributor
Author
|
@SaltyAom ok I think I'm done here lol, I think everything works just as it should. happy to make changes! |
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/utils.ts (1)
168-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the millisecond vs second precision mismatch in your
If-Modified-Sincecomparison~♡You're comparing
fileStats.mtime.getTime()(milliseconds) againstDate.parse(ifModifiedSince)(also milliseconds in JS, but HTTP-dates per RFC 7231 have only second precision). When a client echoes back aLast-Modifiedvalue like1700000000000, and your file's mtime is1700000000123, the check123 <= 0fails—so you keep sending the full file every. single. time. instead of that tasty little304 Not Modified~♡Confirmed: the plugin doesn't currently emit
Last-Modifiedheaders, so this bug rarely bites in practice. But the code path exists, it's tested, and it's still reachable via manually-crafted requests—plus future changes could addLast-Modifiedsupport. Just floor both sides to whole seconds like thefreshpackage does~♡♻️ Suggested fix
if (headers['if-modified-since']) { - const ifModifiedSince = headers['if-modified-since'] + const ifModifiedSince = Date.parse(headers['if-modified-since']) + if (Number.isNaN(ifModifiedSince)) return false + // HTTP-date has 1-second resolution; truncate mtime to match + // so a round-tripped Last-Modified value compares equal. + const mtime = fileStats.mtime?.getTime() + if (mtime === undefined) return false + return Math.floor(mtime / 1000) <= Math.floor(ifModifiedSince / 1000) - try { - return ( - fileStats.mtime !== undefined && - fileStats.mtime.getTime() <= Date.parse(ifModifiedSince) - ) - } catch {} }🤖 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/utils.ts` around lines 168 - 177, The If-Modified-Since check compares millisecond-precision mtime (fileStats.mtime.getTime()) to the parsed header but HTTP dates are second-precision; convert both sides to whole seconds before comparing (e.g., divide or floor both fileStats.mtime.getTime() and Date.parse(ifModifiedSince) by 1000) inside the existing try block that guards headers['if-modified-since'], so replace the direct millisecond comparison in that block with a seconds-based comparison using the same identifiers (headers['if-modified-since'], ifModifiedSince, fileStats.mtime.getTime(), Date.parse(ifModifiedSince)).
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
21-22: ⚡ Quick winEhhh~ No dependency caching? Your CI runs must be sooo slow~ (๑•́ ₃ •̀๑)
Every single CI run is gonna re-download all those packages from scratch~ How inefficient! ♡ Adding a simple cache step would speed things up significantly, but I guess you like waiting around~ (︶︹︺)
⚡ Proposed enhancement to cache dependencies
Add this step before the install step:
- name: Cache dependencies uses: actions/cache@v3 with: path: ~/.bun/install/cache key: ${{ runner.os }}-bun-${{ hashFiles('bun.lockb') }} restore-keys: | ${{ runner.os }}-bun-🤖 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 @.github/workflows/test.yml around lines 21 - 22, Add a dependency caching step before the "Install packages" step to avoid re-downloading packages on every run: create a step named "Cache dependencies" that uses actions/cache@v3, targets the Bun cache directory (e.g., ~/.bun/install/cache) and uses a key like `${{ runner.os }}-bun-${{ hashFiles('bun.lockb') }}` with a restore-keys fallback `${{ runner.os }}-bun-`, placing this step immediately before the step that runs `bun install` (the "Install packages" step).
24-25: ⚡ Quick winNo test artifacts or coverage upload? That's gonna make debugging failures super fun~ (≖ ͜ʖ≖)
When tests fail in CI, you won't have any test reports or coverage data to look at~ You'll just have to stare at the console logs and guess what went wrong~ How thrilling! ♡ But maybeee you could save yourself some headaches by uploading test results and coverage~ Just a thought from someone smarter than you~ ( ˘ ³˘)♡
📊 Proposed enhancement to upload test artifacts
Add this step after the test step:
- name: Upload test results if: always() uses: actions/upload-artifact@v3 with: name: test-results path: | coverage/ test-results/🤖 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 @.github/workflows/test.yml around lines 24 - 25, Add a new GitHub Actions step immediately after the existing "Test" step (the step that runs "bun run test") to always upload test artifacts; use the actions/upload-artifact@v3 action with a descriptive artifact name like "test-results" and include paths such as coverage/ and test-results/ (so coverage and junit/other test reports are preserved) and guard it with if: always() so artifacts upload even on failure.
🤖 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 @.github/workflows/test.yml:
- Around line 16-19: The workflow uses oven-sh/setup-bun with bun-version set to
"latest", which makes CI non-deterministic; update the action input for
bun-version in the GitHub Actions job (the uses: oven-sh/setup-bun@v1 block) to
pin a specific, stable Bun release (e.g., "v1.3.13") instead of "latest" so
builds are reproducible.
---
Outside diff comments:
In `@src/utils.ts`:
- Around line 168-177: The If-Modified-Since check compares
millisecond-precision mtime (fileStats.mtime.getTime()) to the parsed header but
HTTP dates are second-precision; convert both sides to whole seconds before
comparing (e.g., divide or floor both fileStats.mtime.getTime() and
Date.parse(ifModifiedSince) by 1000) inside the existing try block that guards
headers['if-modified-since'], so replace the direct millisecond comparison in
that block with a seconds-based comparison using the same identifiers
(headers['if-modified-since'], ifModifiedSince, fileStats.mtime.getTime(),
Date.parse(ifModifiedSince)).
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 21-22: Add a dependency caching step before the "Install packages"
step to avoid re-downloading packages on every run: create a step named "Cache
dependencies" that uses actions/cache@v3, targets the Bun cache directory (e.g.,
~/.bun/install/cache) and uses a key like `${{ runner.os }}-bun-${{
hashFiles('bun.lockb') }}` with a restore-keys fallback `${{ runner.os }}-bun-`,
placing this step immediately before the step that runs `bun install` (the
"Install packages" step).
- Around line 24-25: Add a new GitHub Actions step immediately after the
existing "Test" step (the step that runs "bun run test") to always upload test
artifacts; use the actions/upload-artifact@v3 action with a descriptive artifact
name like "test-results" and include paths such as coverage/ and test-results/
(so coverage and junit/other test reports are preserved) and guard it with if:
always() so artifacts upload even on failure.
🪄 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: ca670436-84fe-461c-8a9e-b9b21e2fe25e
⛔ Files ignored due to path filters (1)
public/MGICALCURE_LOVE_SHOT.mp3is excluded by!**/*.mp3
📒 Files selected for processing (9)
.github/workflows/test.ymlbench/index.tspackage.jsonsrc/index.tssrc/utils.tstest/index.test.tstest/utils.d.tstsconfig.dts.jsontsconfig.json
✅ Files skipped from review due to trivial changes (2)
- test/utils.d.ts
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- bench/index.ts
- package.json
- test/index.test.ts
- src/index.ts
Comment on lines
+16
to
+19
| - name: Setup bun | ||
| uses: oven-sh/setup-bun@v1 | ||
| with: | ||
| bun-version: latest |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable version of Bun as of May 2026?
💡 Result:
The latest stable Bun version as of May 10, 2026 is Bun v1.3.13.[1][2] Evidence: Bun’s official release landing page and the GitHub “Releases” page show v1.3.13 as the most recent release, dated 2026-04-20 (after which no newer stable release is shown in the results retrieved). [3][4]
Citations:
- 1: https://github.com/oven-sh/bun/releases/tag/bun-v1.3.13
- 2: https://bun.sh/blog/bun-v1.3.13
- 3: https://bun.sh/
- 4: https://github.com/oven-sh/bun/releases
Pin Bun to a specific version instead of latest for deterministic CI builds~
Using latest means your CI runs are non-deterministic and could break randomly when a new Bun version drops♡ How reckless~ (¬‿¬) That's gonna cause all kinds of fun debugging when tests suddenly fail for no reason at all, ne~
The current stable version as of May 2026 is v1.3.13 - pin to that instead of flying blind with latest~
🔧 Proposed fix to pin Bun version
- name: Setup bun
uses: oven-sh/setup-bun@v1
with:
- bun-version: latest
+ bun-version: 1.3.13📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Setup bun | |
| uses: oven-sh/setup-bun@v1 | |
| with: | |
| bun-version: latest | |
| - name: Setup bun | |
| uses: oven-sh/setup-bun@v1 | |
| with: | |
| bun-version: 1.3.13 |
🤖 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 @.github/workflows/test.yml around lines 16 - 19, The workflow uses
oven-sh/setup-bun with bun-version set to "latest", which makes CI
non-deterministic; update the action input for bun-version in the GitHub Actions
job (the uses: oven-sh/setup-bun@v1 block) to pin a specific, stable Bun release
(e.g., "v1.3.13") instead of "latest" so builds are reproducible.
Member
|
ok |
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
4 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 fixes #62 (content-type headers not appearing on non-bun runtimes) by caching file Blobs (along with its content-type) instead of the Response object and using Elysia's own
mapResponsehandler to convert that to a Response. (this gives us the added benefit of range request headers (video files on Bun are now streamable again) and de-duplicated logic for converting files to http responses.)While I was doing this, I also refactored the route mounting logic to consolidate file fetching and caching in one place. All routes are now added with
mountRoute, which takes aurlPathand either a hardcoded file or an absolute file path. If it's the latter, every request on that url will callgetFileResponse, where the file fetching, caching, and etag logic will happen.Here's a quick demo of running the same Vite build on the current version of the plugin (left) vs. the new version (right). Note that JS fails to load on the left version because of the missing content type header. (If you're curious, the website is cmueats.com, which uses Elysia for its backend - I'm considering hosting the frontend on Elysia as well. It's currently on Vercel.)
Screen.Recording.2026-04-29.at.10.30.28.PM.mp4
This PR also fixes several other bugs
alwaysStaticwas true and the number of files exceededstaticLimit, no routes were added at all. Now, at moststaticLimitroutes will be added and the rest will be caught in the wildcard route.indexHTMLon Node previously had no effect.alwaysStaticif clause,shouldIgnore(absolutePath)is checked, rather than the relative path.http://localhost:3001/..%2Fsrc/index.ts). (merged in from fix: resolve wildcard path against assetsDir before read #66)Testing
The test suite now runs on both Bun (through
bun test) and Node (throughvitest). I added tests to cover the above bugs and combinations ofalwaysStatic,bunFullstack, andindexHTML. With regards tostaticLimit, html files will be mounted regardless of the limit iffbunFullstackis true. (this matches current implementation)I also tested my changes on Windows, and everything seems fine.
The existing implementation fails 88 tests on Bun and 96 tests on Node, although these are a bit inflated due to the heavy use of parameterization on my new tests, lol.
Performance
Despite caching Blobs instead of Responses, there doesn't seem to be any performance degregation according to the new
bench/index.tsscript I wrote.Misc
bun dev:nodecommand for starting the example server with node instead of bun.bun run teston every commitThis PR makes #65 redundant
Summary by CodeRabbit
New Features
Tests