Fix offline page service worker behavior - #104
Conversation
Harden service-worker installation so failed precache assets do not activate a broken offline worker, redirect bare /sl offline navigations to the canonical shell URL, and add Playwright validation for the offline page flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves apps/page offline behavior by making service worker installation fail on precache errors, adding an offline navigation redirect for the bare project path (/sl -> /sl/), and introducing Playwright E2E coverage to validate offline behavior end-to-end.
Changes:
- Make service worker install reject when required assets fail to precache, and add offline redirect handling for bare base-path navigations.
- Add unit coverage for the service worker regression scenarios and navigation redirect helper.
- Add Playwright-based offline E2E test infrastructure (config + local Bun server) and expose an e2e test script.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds a root test:page:e2e script to run the page E2E suite. |
| bun.lock | Adds Playwright dependencies (@playwright/test, playwright, playwright-core). |
| apps/page/src/service-worker.ts | Updates install/activate/fetch logic: fail install on precache error and redirect /sl to /sl/ when offline. |
| apps/page/src/service-worker.test.ts | Adds regression tests for install failure behavior and offline /sl redirect behavior. |
| apps/page/src/service-worker-navigation.ts | Adds getNavigationRedirectPath() helper for canonical trailing-slash redirects. |
| apps/page/src/service-worker-navigation.test.ts | Adds unit tests for getNavigationRedirectPath(). |
| apps/page/src/service-worker-build.test.ts | Adjusts the regex assertion formatting for the WASM injection build test. |
| apps/page/playwright/offline.e2e.js | Adds an E2E scenario validating offline load from the bare project path without leaking to root assets. |
| apps/page/playwright.config.mjs | Introduces Playwright configuration and a webServer hook for E2E execution. |
| apps/page/playwright-server.mjs | Adds a Bun static file server used by Playwright E2E tests (serving under /sl). |
| apps/page/package.json | Adds Playwright scripts and dependencies; scopes unit tests to ./src and adds test:e2e. |
| // Claim all clients immediately | ||
| self.clients.claim(); | ||
| serviceWorker.clients.claim(); |
There was a problem hiding this comment.
clients.claim() is invoked outside of event.waitUntil(...). If the activation event finishes before the promise resolves, the browser can terminate the task early and the new service worker may not reliably take control of existing pages. Include the clients.claim() promise in the waitUntil chain (e.g., as part of the final Promise.all).
See below for a potential fix:
// Claim all clients immediately, but keep it within waitUntil so activation
// is not finished before the claim promise resolves.
return Promise.all([deleteOldCaches, cleanupStaleAssets, serviceWorker.clients.claim()]);
})
);
| if (!pathname.startsWith(`${basePath}/`)) { | ||
| return null; | ||
| } | ||
|
|
||
| return path.join(libDir, pathname.slice(`${basePath}/`.length)); | ||
| }; |
There was a problem hiding this comment.
resolvePathname() uses path.join(libDir, pathname.slice(...)) directly from the request URL. A crafted path containing .. segments can escape libDir via path traversal (even if this server is only used for E2E tests). Consider normalizing and rejecting paths with .. (or verifying the resolved path still starts with libDir + path.sep) before serving the file.
|
@copilot fix the nsis install |
…al in playwright-server Agent-Logs-Url: https://github.com/scaryrawr/sl/sessions/b77534e5-720a-4bd0-b2ff-bb5684bc17a5 Co-authored-by: scaryrawr <661373+scaryrawr@users.noreply.github.com>
Agent-Logs-Url: https://github.com/scaryrawr/sl/sessions/b77534e5-720a-4bd0-b2ff-bb5684bc17a5 Co-authored-by: scaryrawr <661373+scaryrawr@users.noreply.github.com>
Fixed in commit 4d4cc14. Replaced |
|
@copilot can we get some screenshots of offline and online working? We should expect an ascii art train |
Here are the screenshots showing the ASCII art train working in both states: Online — page loads normally, train enters from the right: Offline — server stopped, service worker serves all assets from cache and the train keeps running: |
…a-4cee-aaf5-8cf8c215bccd Co-authored-by: scaryrawr <661373+scaryrawr@users.noreply.github.com>
Summary
Testing