fix(ui): always give respondWith a real Response in the sw fetch fallback (#11292)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - The UI registers a service worker (`ui/public/sw.js`) with a
network-first fetch handler whose cache is an offline fallback
> - The fallback hands `event.respondWith` the result of
`caches.match(...)`, which resolves `undefined` on a cache miss — and in
the navigation branch, `caches.match("/") || offlineResponse` never uses
the fallback because `caches.match` returns a promise, which is always
truthy
> - When the network fetch rejects (server restart, deploy, brief
outage) and the cache misses, the browser fails the request with
`Uncaught (in promise) TypeError: Failed to convert value to
'Response'`, so navigation breaks outright instead of degrading to the
offline page
> - This pull request awaits the cache lookups and guarantees a real
`Response` on every path
> - The benefit is that brief server unavailability degrades to the
offline fallback instead of a dead navigation
## Linked Issues or Issue Description
No existing issue. Description follows the bug template:
**What happened?**
Navigating while the server was briefly unavailable (mid-restart)
produced `The FetchEvent for "…" resulted in a network error response:
the promise was rejected.` and `sw.js:1 Uncaught (in promise) TypeError:
Failed to convert value to 'Response'.` The navigation failed instead of
showing the offline fallback.
**Expected behavior**
A failed navigation serves the cached app shell when present, otherwise
the "Offline" 503 response. A failed asset fetch serves its cache entry
when present, otherwise a proper network-error response. `respondWith`
always receives a real `Response`.
**Steps to reproduce**
1. Load the app so `sw.js` is active; ensure `/` is not in the service
worker cache (fresh cache version).
2. Restart or stop the backend.
3. Navigate to any page: the fetch rejects, `caches.match` misses, and
the browser logs the conversion TypeError with a failed navigation.
## What Changed
- `ui/public/sw.js`: the fetch fallback awaits `caches.match(...)` and
returns the "Offline" 503 for navigations and `Response.error()` for
assets when the cache misses.
- `ui/src/lib/sw-offline-fallback.test.ts`: evaluates the real `sw.js`
in a sandboxed scope and covers the three fallback paths; the two
miss-path tests fail against the previous code.
## Verification
- `pnpm vitest run src/lib/sw-offline-fallback.test.ts` in `ui/` — 3
tests pass.
- Verified both miss-path tests fail against the unmodified `sw.js`.
## Risks
Low risk. The change only affects the fetch-rejection path; successful
fetches and cache hits behave exactly as before. `Response.error()`
mirrors what the browser would produce for an unhandled failed no-cors
fetch.
## Model Used
- Claude (Anthropic), Claude Fable 5 (`claude-fable-5`), via Claude Code
CLI with tool use (code search, edit, test execution).
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge
This commit is contained in:
parent
2c53437fc9
commit
01112c350c
|
|
@ -32,11 +32,16 @@ self.addEventListener("fetch", (event) => {
|
|||
}
|
||||
return response;
|
||||
})
|
||||
.catch(() => {
|
||||
.catch(async () => {
|
||||
// caches.match() resolves undefined on a miss (and the promise itself
|
||||
// is always truthy, so `||` can never supply a fallback). respondWith
|
||||
// must always receive a real Response — resolving undefined breaks
|
||||
// the navigation with "Failed to convert value to 'Response'" instead
|
||||
// of showing anything.
|
||||
if (request.mode === "navigate") {
|
||||
return caches.match("/") || new Response("Offline", { status: 503 });
|
||||
return (await caches.match("/")) ?? new Response("Offline", { status: 503 });
|
||||
}
|
||||
return caches.match(request);
|
||||
return (await caches.match(request)) ?? Response.error();
|
||||
})
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -0,0 +1,110 @@
|
|||
import { readFileSync } from "node:fs";
|
||||
import { resolve } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
|
||||
const uiRoot = resolve(fileURLToPath(new URL("../..", import.meta.url)));
|
||||
|
||||
type FetchListener = (event: {
|
||||
request: { method: string; url: string; mode: string };
|
||||
respondWith: (response: Promise<Response | undefined> | Response) => void;
|
||||
}) => void;
|
||||
|
||||
function loadServiceWorkerFetchListener(overrides: {
|
||||
fetch: () => Promise<Response>;
|
||||
cachesMatch: (key: unknown) => Promise<Response | undefined>;
|
||||
}): FetchListener {
|
||||
const listeners = new Map<string, (event: unknown) => void>();
|
||||
const swSelf = {
|
||||
addEventListener: (type: string, listener: (event: unknown) => void) => {
|
||||
listeners.set(type, listener);
|
||||
},
|
||||
skipWaiting: vi.fn(),
|
||||
clients: { claim: vi.fn() },
|
||||
location: { origin: "https://app.example.com" },
|
||||
};
|
||||
const caches = {
|
||||
match: overrides.cachesMatch,
|
||||
open: vi.fn(async () => ({ put: vi.fn() })),
|
||||
keys: vi.fn(async () => []),
|
||||
delete: vi.fn(async () => true),
|
||||
};
|
||||
const code = readFileSync(resolve(uiRoot, "public/sw.js"), "utf8");
|
||||
new Function("self", "caches", "fetch", "Response", "URL", code)(
|
||||
swSelf,
|
||||
caches,
|
||||
overrides.fetch,
|
||||
Response,
|
||||
URL,
|
||||
);
|
||||
const listener = listeners.get("fetch");
|
||||
if (!listener) throw new Error("sw.js registered no fetch listener");
|
||||
return listener as FetchListener;
|
||||
}
|
||||
|
||||
async function respondTo(
|
||||
listener: FetchListener,
|
||||
request: { method: string; url: string; mode: string },
|
||||
): Promise<Response | undefined> {
|
||||
let captured: Promise<Response | undefined> | Response | undefined;
|
||||
listener({
|
||||
request,
|
||||
respondWith: (response) => {
|
||||
captured = response;
|
||||
},
|
||||
});
|
||||
return await captured;
|
||||
}
|
||||
|
||||
describe("sw.js offline fallback", () => {
|
||||
it("serves the Offline response for a failed navigation with an empty cache", async () => {
|
||||
const listener = loadServiceWorkerFetchListener({
|
||||
fetch: () => Promise.reject(new TypeError("network down")),
|
||||
cachesMatch: async () => undefined,
|
||||
});
|
||||
|
||||
const response = await respondTo(listener, {
|
||||
method: "GET",
|
||||
url: "https://app.example.com/settings/instance",
|
||||
mode: "navigate",
|
||||
});
|
||||
|
||||
// respondWith must always receive a real Response; resolving undefined
|
||||
// fails the navigation with "Failed to convert value to 'Response'".
|
||||
expect(response).toBeInstanceOf(Response);
|
||||
expect(response!.status).toBe(503);
|
||||
expect(await response!.text()).toBe("Offline");
|
||||
});
|
||||
|
||||
it("serves the cached shell for a failed navigation when one exists", async () => {
|
||||
const shell = new Response("<html>app shell</html>", { status: 200 });
|
||||
const listener = loadServiceWorkerFetchListener({
|
||||
fetch: () => Promise.reject(new TypeError("network down")),
|
||||
cachesMatch: async (key) => (key === "/" ? shell : undefined),
|
||||
});
|
||||
|
||||
const response = await respondTo(listener, {
|
||||
method: "GET",
|
||||
url: "https://app.example.com/settings/instance",
|
||||
mode: "navigate",
|
||||
});
|
||||
|
||||
expect(response).toBe(shell);
|
||||
});
|
||||
|
||||
it("returns a network-error Response for a failed asset with no cache entry", async () => {
|
||||
const listener = loadServiceWorkerFetchListener({
|
||||
fetch: () => Promise.reject(new TypeError("network down")),
|
||||
cachesMatch: async () => undefined,
|
||||
});
|
||||
|
||||
const response = await respondTo(listener, {
|
||||
method: "GET",
|
||||
url: "https://app.example.com/assets/index-abc.js",
|
||||
mode: "no-cors",
|
||||
});
|
||||
|
||||
expect(response).toBeInstanceOf(Response);
|
||||
expect(response!.type).toBe("error");
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue