Conversation
✅ Deploy Preview for dfsrhrhrghdrf ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the service worker and client bootstrap script formatting, updates server routing/initialization in index.js, fixes a quoting issue in start.sh, and bumps the Docker Node base image.
Changes:
- Rewrote
static/sw.jsinto a readable service worker with a new pre-cache list and simplified failure handling. - Updated
index.jsto adjust routes/redirects, fix ESM__dirnamehandling, and remove an unused import. - Bumped Docker base image to Node 20 and fixed a broken
echoline instart.sh.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| static/sw.js | Refactor SW implementation and update pre-cache URL list. |
| static/assets/js/main.js | Formatting changes plus new client-side redirect behavior. |
| start.sh | Fix broken quoting in echo output. |
| index.js | Adjust Express setup, routes, redirects, and ESM path handling. |
| Dockerfile | Update Node base image version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (window.location.pathname === '/index.html' || window.location.pathname === '/') { | ||
| location.href = '/math.html'; | ||
| } | ||
|
|
There was a problem hiding this comment.
The client-side redirect from '/' or '/index.html' to '/math.html' makes the index page effectively unreachable (and runs index-only UI setup work that users never see). If the intent is to change the landing page, consider doing this redirect server-side (or removing it) and ensure the service worker pre-cache includes '/math.html' so the first navigation still works offline.
| if (window.location.pathname === '/index.html' || window.location.pathname === '/') { | |
| location.href = '/math.html'; | |
| } |
| const CACHE_NAME = 'arctic-v1.0'; | ||
| const urlsToCache = [ | ||
| '/', | ||
| '/index.html', |
There was a problem hiding this comment.
The pre-cache list doesn't include '/math.html', but the app now redirects users there from '/' and '/index.html'. This can cause a broken/offline experience (and even an extra network round-trip online) since the redirected-to document isn't guaranteed to be cached.
| '/index.html', | |
| '/index.html', | |
| '/math.html', |
| '/assets/css/app.css', | ||
| '/assets/css/menu.css', | ||
| '/assets/js/particles.js', | ||
| '/assets/js/themes.js', | ||
| '/assets/js/index.js', |
There was a problem hiding this comment.
The HTML requests versioned assets (e.g. /assets/css/app.css?v=3), but the pre-cache list uses unversioned URLs. Since caches.match(event.request) matches on the full URL including query params, these pre-cached entries won't satisfy the versioned requests. Consider caching the exact versioned URLs or adjusting the matching strategy (e.g. ignoreSearch) if appropriate.
| '/assets/css/menu.css', | ||
| '/assets/js/particles.js', | ||
| '/assets/js/themes.js', | ||
| '/assets/js/index.js', | ||
| '/assets/js/anym.js', | ||
| '/assets/js/main.js', |
There was a problem hiding this comment.
The service worker pre-caches '/settings.html' but not its required assets (notably '/assets/css/settings.css?v=3' and '/assets/js/settings.js?v=3'), so the settings page won't work offline even though the document is cached. Consider adding those dependencies to urlsToCache (matching the versioned URLs used in the HTML).
| '/assets/css/menu.css', | |
| '/assets/js/particles.js', | |
| '/assets/js/themes.js', | |
| '/assets/js/index.js', | |
| '/assets/js/anym.js', | |
| '/assets/js/main.js', | |
| '/assets/css/menu.css', | |
| '/assets/css/settings.css?v=3', | |
| '/assets/js/particles.js', | |
| '/assets/js/themes.js', | |
| '/assets/js/index.js', | |
| '/assets/js/anym.js', | |
| '/assets/js/main.js', | |
| '/assets/js/settings.js?v=3', |
| import { fileURLToPath } from 'node:url'; | ||
| import { createRequire } from 'node:module'; | ||
| import { epoxyPath } from '@mercuryworkshop/epoxy-transport'; | ||
| import { baremuxPath } from '@mercuryworkshop/bare-mux/node'; | ||
| import { createBareServer } from '@tomphttp/bare-server-node'; |
There was a problem hiding this comment.
@cypress/request is no longer imported anywhere in the codebase, but it remains in package.json dependencies. Consider removing it to reduce install size and avoid carrying an unused dependency.
let's test