fix(service-worker): respondWith synchronously and drop extension bypass - #226
Conversation
…n bypass F6: respondWith() was called after an await (past the dispatch flag), so it threw InvalidStateError on every request and all traffic fell through to the network. Call respondWith() synchronously with a promise that runs the srvx handler and, on a 404, falls back to fetch(event.request). Also fix the window branch reload loop: only reload once the freshly installed worker takes control (controllerchange) instead of whenever an active registration exists. F28: remove the /\/[^/]*\.[a-zA-Z0-9]+$/ extension-bypass regex so the handler sees every same-origin request (e.g. /api/data.json); unhandled 404s now fall back to the network inside the response promise. Add in-process tests for the fetch-listener logic and update the example so the worker script loads from the network. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesService worker routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/service-worker.test.ts (1)
14-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the browser-window reload/controllerchange fix.
windowis stubbed toundefined, so only theisServiceWorkerbranch (service-worker.ts Lines 87-103) is exercised. The other headline fix in this PR — the one-shotcontrollerchangereload in service-worker.ts Lines 71-86 — has no test in this suite. Consider adding a case that stubsnavigator.serviceWorker(controller unset → register → controllerchange → reload called once) to lock in that fix.🤖 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/service-worker.test.ts` around lines 14 - 44, Add a test covering the browser-window branch in the service-worker adapter setup: stub navigator.serviceWorker with no initial controller, trigger registration followed by controllerchange, and assert the reload handler is called exactly once. Keep the existing service-worker branch test intact and ensure the new case exercises the one-shot behavior.
🤖 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/adapters/service-worker.ts`:
- Around line 97-100: The fetch path in the service-worker adapter
unconditionally replaces every handler 404 with a network request, preventing
intentional 404 responses from reaching clients. Update the logic around
this.fetch and the response.status check to require an explicit network-fallback
opt-in or sentinel marker before falling back, while preserving ordinary handler
responses—including deliberate 404s—by default.
- Around line 89-103: Update the this.#fetchListener handler to clone
event.request before passing it to this.fetch, while retaining the original
event.request for the 404 fallback fetch. Ensure the cloned request is used only
for handler invocation and the existing response-status behavior remains
unchanged.
---
Nitpick comments:
In `@test/service-worker.test.ts`:
- Around line 14-44: Add a test covering the browser-window branch in the
service-worker adapter setup: stub navigator.serviceWorker with no initial
controller, trigger registration followed by controllerchange, and assert the
reload handler is called exactly once. Keep the existing service-worker branch
test intact and ensure the new case exercises the one-shot behavior.
🪄 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: 2b3d56d6-e79b-4d5a-811a-3aaef4ac1c9a
📒 Files selected for processing (3)
examples/service-worker/public/sw.mjssrc/adapters/service-worker.tstest/service-worker.test.ts
| this.#fetchListener = (event) => { | ||
| Object.defineProperty(event.request, "waitUntil", { | ||
| value: event.waitUntil.bind(event), | ||
| }); | ||
| const response = await this.fetch(event.request, event); | ||
| if (response.status !== 404) { | ||
| event.respondWith(response); | ||
| } | ||
| // `respondWith` must be called synchronously (before the event | ||
| // dispatch completes), passing a promise that resolves the response. | ||
| event.respondWith( | ||
| (async () => { | ||
| const response = await this.fetch(event.request, event); | ||
| // Treat a 404 from the handler as "not handled" and fall back | ||
| // to the network for the original request. | ||
| return response.status === 404 ? fetch(event.request) : response; | ||
| })(), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== service-worker.ts ==\n'
sed -n '1,220p' src/adapters/service-worker.ts
printf '\n== search for fetch implementation and request cloning ==\n'
rg -n "class .*fetch|fetch\\(request|request\\.clone\\(|clone\\(\\)" src -g '!**/*.map'
printf '\n== likely adapter files ==\n'
fd -a "service-worker.ts|fetch.*ts|adapter.*ts" srcRepository: h3js/srvx
Length of output: 4816
🏁 Script executed:
echo hiRepository: h3js/srvx
Length of output: 150
🏁 Script executed:
set -euo pipefail
printf '\n== _middleware.ts outline ==\n'
ast-grep outline src/_middleware.ts --view expanded
printf '\n== _middleware.ts contents ==\n'
sed -n '1,260p' src/_middleware.ts
printf '\n== search for request/body handling in src ==\n'
rg -n "request\\.(json|text|arrayBuffer|formData)|clone\\(\\)|new Request\\(" srcRepository: h3js/srvx
Length of output: 1786
🏁 Script executed:
set -euo pipefail
sed -n '1,260p' src/_middleware.tsRepository: h3js/srvx
Length of output: 881
Clone the request before invoking the handler
src/adapters/service-worker.ts:89-103 this.fetch(...) and the 404 fallback both use the same Request. If middleware or the handler reads a POST/PUT/PATCH body before returning 404, fetch(event.request) reuses a consumed stream and throws instead of falling back to the network. Clone the request for the handler and keep the original for fallback.
🤖 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/adapters/service-worker.ts` around lines 89 - 103, Update the
this.#fetchListener handler to clone event.request before passing it to
this.fetch, while retaining the original event.request for the 404 fallback
fetch. Ensure the cloned request is used only for handler invocation and the
existing response-status behavior remains unchanged.
| const response = await this.fetch(event.request, event); | ||
| // Treat a 404 from the handler as "not handled" and fall back | ||
| // to the network for the original request. | ||
| return response.status === 404 ? fetch(event.request) : response; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
HTTP 404 is overloaded as "not handled" — legitimate 404 responses from handlers are unrecoverably replaced.
Any handler response with status 404 is discarded in favor of a raw network fetch (Line 100), even when the 404 is a deliberate response (e.g. a JSON "not found" body from an API route). Applications with real API routes can never surface an intentional 404 to the client via this adapter — it's silently swapped for whatever the network returns for that URL.
Since this is explicitly documented as the intended routing-fallback behavior, consider gating it behind an explicit opt-in (e.g. a serviceWorker.networkFallback option or a dedicated sentinel header/marker) rather than keying off any 404 response unconditionally, so handlers that legitimately want to return 404 aren't silently overridden.
🤖 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/adapters/service-worker.ts` around lines 97 - 100, The fetch path in the
service-worker adapter unconditionally replaces every handler 404 with a network
request, preventing intentional 404 responses from reaching clients. Update the
logic around this.fetch and the response.status check to require an explicit
network-fallback opt-in or sentinel marker before falling back, while preserving
ordinary handler responses—including deliberate 404s—by default.
Fixes two locked-together bugs in the service-worker adapter.
event.respondWith()was called after anawait, past the dispatch flag, throwingInvalidStateErroron every request so all traffic fell through to the network; the window branch also reloaded whenever an active registration existed, causing an infinite reload loop. NowrespondWith()is called synchronously with a promise, and the window branch only reloads on firstcontrollerchange./\/[^/]*\.[a-zA-Z0-9]+$/extension-bypass regex; the handler now sees every same-origin request (e.g./api/data.json), and an unhandled 404 falls back to the network inside the response promise.Adds in-process tests for the fetch-listener logic and updates
examples/service-workerso the worker script loads from the network.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes