feat: add enableFallback option to allow route fallback #50
Conversation
Add enableFallback option that allows requests to fall through to other route handlers when static files are not found, instead of returning 404. This enables use cases where static file serving should not intercept other routes like .all() or API endpoints.
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Plugin as StaticPlugin
participant FS as FileSystem
participant Next as NextRoute
Client->>Plugin: HTTP request (method, path)
Plugin->>FS: resolve path (decode URI, strip prefix), check index.html
alt file exists
FS-->>Plugin: file bytes + stats
Plugin->>Plugin: compute/lookup ETag in fileCache
Plugin-->>Client: 200 + body + Cache-Control + ETag
else file missing
alt enableFallback = true AND method is GET/HEAD
Plugin->>Next: fall through to next route (silently)
Note right of Next: next handler may respond or return 404
Next-->>Client: next handler response
else
Plugin-->>Client: 404 / NotFound (per-route behavior)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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
🧹 Nitpick comments (3)
src/types.ts (1)
120-129: Document scope: enableFallback has no effect with alwaysStaticClarify in the JSDoc that enableFallback only applies when alwaysStatic is false (dynamic mode). Otherwise it’s ignored.
src/index.ts (1)
241-294: Preserve headers on 304 responses from serveStaticFileWhen returning 304, include initialHeaders to match behavior elsewhere and avoid header regressions.
Apply this diff:
- if (requestHeaders && etag && (await isCached(requestHeaders, etag, pathName))) - return new Response(null, { status: 304 }) + if (requestHeaders && etag && (await isCached(requestHeaders, etag, pathName))) + return new Response(null, { + status: 304, + headers: isNotEmpty(initialHeaders) ? initialHeaders : undefined + })test/index.test.ts (1)
446-498: Nice coverage; consider adding cache and method tests for fallback
- Add a test asserting 304 with If-None-Match when enableFallback is true (to prevent regressions).
- Add a test that POST to a static path still 404s when enableFallback is true.
Apply this diff to extend tests:
@@ it('should fallback to other routes when enableFallback is true', async () => { @@ expect(staticRes.status).toBe(200) }) + it('should return 304 with If-None-Match when enableFallback is true', async () => { + const app = new Elysia().use( + staticPlugin({ + prefix: '/', + enableFallback: true + }) + ) + await app.modules + // First fetch to get current ETag + const first = await app.handle(req('/takodachi.png')) + const etag = first.headers.get('Etag') + expect(etag).toBeTruthy() + const r = req('/takodachi.png') + if (etag) r.headers.append('If-None-Match', etag) + const second = await app.handle(r) + expect(second.status).toBe(304) + }) + + it('should not serve static content for POST when enableFallback is true', async () => { + const app = new Elysia().use( + staticPlugin({ + prefix: '/', + enableFallback: true + }) + ) + await app.modules + const postReq = new Request('http://localhost/takodachi.png', { method: 'POST' }) + const res = await app.handle(postReq) + expect(res.status).toBe(404) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/index.ts(2 hunks)src/types.ts(1 hunks)src/utils.ts(0 hunks)test/index.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (1)
src/utils.ts (6)
getFile(193-198)isBun(6-6)fileExists(70-77)isNotEmpty(217-223)generateETag(200-215)isCached(137-189)
test/index.test.ts (2)
src/index.ts (1)
staticPlugin(19-350)test/utils.ts (1)
req(1-1)
🔇 Additional comments (1)
src/index.ts (1)
32-34: API addition looks goodDefault enableFallback = false matches the types and preserves backward compatibility.
- Forward request headers to serveStaticFile to enable 304 caching - Restrict static file serving to GET/HEAD methods only - Prevents unexpected behavior with POST/PUT/DELETE requests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.ts (1)
296-328: Previous feedback addressed: method guard + header forwarding addedThe fallback now limits to GET/HEAD and forwards request headers into serveStaticFile, fixing the caching/method issues raised earlier. Nice.
🧹 Nitpick comments (2)
src/index.ts (2)
241-294: 304 should include cache headers; also cache responses when useETag=false
- 304 responses currently omit Cache-Control/ETag, reducing cache correctness.
- When useETag=false, responses aren’t added to fileCache, causing unnecessary reads.
Apply this diff:
- if (!useETag) - return new Response( - file, - isNotEmpty(initialHeaders) - ? { headers: initialHeaders } - : undefined - ) + if (!useETag) { + const response = new Response( + file, + isNotEmpty(initialHeaders) + ? { headers: initialHeaders } + : undefined + ) + fileCache.set(pathName, response) + return response.clone() + } @@ - if (requestHeaders && etag && (await isCached(requestHeaders, etag, pathName))) - return new Response(null, { status: 304 }) + if ( + requestHeaders && + etag && + (await isCached(requestHeaders, etag, pathName)) + ) + return new Response(null, { + status: 304, + headers: Object.assign( + { + 'Cache-Control': maxAge + ? `${directive}, max-age=${maxAge}` + : directive + }, + initialHeaders, + etag ? { Etag: etag } : {} + ) + })
296-304: Optional: Avoid sending a body on HEAD requestsTo better match semantics, return a no-body response for HEAD (same status/headers as GET path).
Would you like a small patch that wraps serveStaticFile’s response for HEAD by stripping the body?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
src/utils.ts (6)
getFile(193-198)isBun(6-6)fileExists(70-77)isNotEmpty(217-223)generateETag(200-215)isCached(137-189)
🔇 Additional comments (1)
src/index.ts (1)
32-34: API option looks goodAdding enableFallback with default false preserves backward compatibility. LGTM.
- Use path.resolve() to get absolute paths and enforce containment - Check that resolved paths stay within assetsDir boundary - Convert to relative paths when checking ignorePatterns - Apply to both enableFallback and non-fallback modes - Add tests to verify directory traversal protection
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/index.test.ts (1)
519-541: Add HEAD and 304 cache tests for fallback pathTo lock in method guard and caching:
- Assert HEAD on static path returns 200 with no body.
- Assert 304 is returned when If-None-Match/If-Modified-Since are present under enableFallback.
Also consider a case with enableFallback: true and prefix: '/public' where '/takodachi.png' is 404 but '/public/takodachi.png' is 200, to verify prefix strip logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(2 hunks)test/index.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (1)
src/utils.ts (6)
getFile(193-198)isBun(6-6)fileExists(70-77)isNotEmpty(217-223)generateETag(200-215)isCached(137-189)
test/index.test.ts (2)
src/index.ts (1)
staticPlugin(19-375)test/utils.ts (1)
req(1-1)
🔇 Additional comments (5)
test/index.test.ts (3)
446-463: Fallback behavior test (enableFallback: true) looks goodCovers the core regression: API routes win; static file served at root. Nice.
484-498: .all() interop test is solidConfirms route pattern precedence under fallback. Good coverage.
500-517: Traversal protection (default mode) verifiedGood set of cases, including encoded “..”. Matches path containment checks.
src/index.ts (2)
296-337: Fallback handler: method guard, prefix strip, traversal, and header forwarding — LGTMGET/HEAD check prevents serving on mutating methods; prefix handling and containment checks look right; request headers are forwarded to preserve 304 behavior.
338-371: Route-based handler: traversal enforcement and shared serving path — LGTMMirrors fallback logic with NotFoundError semantics. Header passthrough enables 304s. Looks correct.
- Use relative path for shouldIgnore check instead of absolute path - Include initialHeaders in 304 responses for consistency - Use actual file path (index.html) for isCached check instead of directory path
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/index.ts (1)
241-304: Cache key should use targetPath; include standard ETag on 304 and 200When serving a directory index.html, you compute targetPath but still cache under pathName. Cache under targetPath to ensure future hits and consistency. Also, return ETag on 304 and use the standard header casing "ETag" in success responses.
Apply this diff:
@@ - const etag = await generateETag(file) - if (requestHeaders && etag && (await isCached(requestHeaders, etag, targetPath))) - return new Response(null, { - status: 304, - headers: isNotEmpty(initialHeaders) ? initialHeaders : undefined - }) + const etag = await generateETag(file) + if (requestHeaders && etag && (await isCached(requestHeaders, etag, targetPath))) + return new Response(null, { + status: 304, + headers: Object.assign({}, initialHeaders, etag ? { ETag: etag } : {}) + }) @@ - const response = new Response(file, { + const response = new Response(file, { headers: Object.assign( { 'Cache-Control': maxAge ? `${directive}, max-age=${maxAge}` : directive }, initialHeaders, - etag ? { Etag: etag } : {} + etag ? { ETag: etag } : {} ) }) @@ - fileCache.set(pathName, response) + fileCache.set(targetPath, response) return response.clone()Optional: also add Last-Modified from fileStat to 200 responses to enable If-Modified-Since flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
src/utils.ts (6)
getFile(193-198)isBun(6-6)fileExists(70-77)isNotEmpty(217-223)generateETag(200-215)isCached(137-189)
🔇 Additional comments (3)
src/index.ts (3)
32-34: Option addition looks goodDefaults preserve backward compatibility; no concerns.
306-347: Global fallback path: method guard + header forwarding LGTMGET/HEAD restriction, prefix gating, traversal protection, and forwarding headers to honor 304s look correct.
348-381: The review comment is based on incorrect assumptions and should be disregarded.Elysia (v1.4+, which this project uses) automatically normalizes HTTP header names to lowercase in
ctx.headers. TheisCachedfunction correctly expects lowercase keys ('if-none-match','cache-control','if-modified-since'), and it receives them already normalized—no additional normalization needed.Additionally, Elysia v1.4+ automatically handles HEAD requests for GET routes; a separate HEAD route handler is not required.
The code at lines 348–381 works correctly as-is.
Likely an incorrect or invalid review comment.
1 participant
This PR adds an
enableFallbackoption that allows requests to fall through to other route handlers when static files are not found, instead of immediately returning a 404 error.Problem: When using the static plugin with
prefix: '/', other routes like.all('/api/auth/*')were being intercepted by the wildcard static route, causing them to return 404.Solution: Added an
enableFallbackoption that, when set totrue, usesonError({ as: 'global' })to handle 404s globally and serve static files only after other routes have been tried.Usage Example:
Changes:
enableFallbackoption toStaticOptionsinterface (defaults tofalse)serveStaticFilehelper function to avoid code duplicationenableFallbackistrue, uses global error handler instead of registering a wildcard routeBackward Compatibility: This change is fully backward compatible. The default behavior (
enableFallback: false) remains unchanged.P.S. This PR also includes:
bun.lock(changes that appeared when runningbun installon main branch)import type { Stats } from 'fs'fromsrc/index.tsandsrc/utils.tsSummary by CodeRabbit
New Features
Tests
Chores