Add PWA install prompt banner and service worker#185
Open
Coteh wants to merge 31 commits into
Open
Conversation
- Add sw.js (ported from offline branch) with cache-first strategy and versioned cache names for full PWA installability on Chrome/Android - Add scripts/transform_sw.sh to inject git commit hash into sw.js at build time, and wire it into scripts/build.sh - Add slide-up install banner at bottom of page that appears after 1.5s on first visit (permanently dismissed via localStorage) - On non-iOS: defers beforeinstallprompt and triggers native install dialog - On iOS: shows manual "Tap share then Add to Home Screen" instruction - Banner does not appear if app is already running in standalone mode https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a PWA install banner (UI + client lifecycle), swaps Feather for Lucide icons, adds a versioned service worker with precaching and commit hash injection during build, and introduces comprehensive Cypress tests for the install banner and related icon loading adjustments. Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser/User
participant App as Web App (index)
participant Banner as Install Banner UI
participant SW as Service Worker
participant Cache as Cache Storage
participant Network as Network
Browser->>App: Load page
App->>SW: registerServiceWorker()
App->>Banner: initInstallBanner()
Browser-->>App: beforeinstallprompt (optional)
App->>Banner: show banner (if allowed & not dismissed)
Browser->>Banner: User clicks Install
Banner->>Browser: prompt.prompt()
Browser->>Browser: user accepts/dismisses
SW->>Cache: install -> addResourcesToCache(PRECACHE_URLS)
SW->>SW: activate -> remove old caches
Browser->>Network: request resource
SW->>Cache: cacheFirst(request)
alt cached
Cache-->>SW: return cached response
SW-->>Browser: serve cached response
else not cached
SW->>Network: fetch(request)
Network-->>SW: response
SW->>Cache: putInCache(request, response)
SW-->>Browser: serve network response
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 5
🧹 Nitpick comments (5)
scripts/build.sh (1)
55-56: Quote the$OUTPUT_DIRvariable for robustness.For consistency with other parts of the build script and to handle paths with spaces, consider quoting the variable.
Suggested fix
-./scripts/transform_sw.sh $OUTPUT_DIR +./scripts/transform_sw.sh "$OUTPUT_DIR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build.sh` around lines 55 - 56, The call to scripts/transform_sw.sh uses the unquoted $OUTPUT_DIR which will break on paths with spaces; update the invocation of scripts/transform_sw.sh to pass "$OUTPUT_DIR" (i.e., quote the $OUTPUT_DIR variable) so the script receives the full path correctly and matches quoting convention used elsewhere in the build script.index.css (1)
741-759: Consider adding focus/hover states for accessibility.The install and dismiss buttons lack visible
:focusand:hoverstates. This is important for keyboard navigation and overall accessibility.Suggested addition
.install-banner-btn:hover, .install-banner-btn:focus { opacity: 0.85; outline: 2px solid var(--text-color); outline-offset: 2px; } .install-banner-dismiss:hover, .install-banner-dismiss:focus { opacity: 0.7; outline: 2px solid var(--text-color); outline-offset: 2px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.css` around lines 741 - 759, Add visible hover and focus states for .install-banner-btn and .install-banner-dismiss to improve keyboard accessibility: implement :hover and :focus (or :focus-visible) rules that change opacity and add a clear outline (with outline-offset) using a contrasting variable like --text-color, ensuring the install button and dismiss button both show distinct visual focus and hover cues while preserving existing padding, border-radius and colors.src/index.js (2)
587-599: Consider deferring service worker registration until after page load.Registering the service worker immediately at script execution can delay initial page render. Deferring to
loadevent improves first-paint performance.Suggested approach
-registerServiceWorker(); +window.addEventListener("load", registerServiceWorker);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 587 - 599, The service worker is being registered immediately which can impact first-paint; modify the flow so registerServiceWorker() is invoked on the window "load" event instead of calling it at script execution. Update the code that calls registerServiceWorker() so it adds a one-time listener (e.g., window.addEventListener("load", ...) or check document.readyState) and then calls registerServiceWorker(), ensuring registerServiceWorker (and its use of navigator.serviceWorker.register("/sw.js", { scope: "/" })) remains unchanged and only runs after the page has fully loaded.
522-585:initInstallBannermay execute before DOM is ready.This function is invoked at module load time (line 585) outside of
DOMContentLoaded. Since it accesses DOM elements viagetElementById, it could fail if the script loads before the banner element exists. The script tag inindex.htmlis placed at the end of<body>, so this should work in practice, but it's fragile.Consider moving the
initInstallBanner()call inside theDOMContentLoadedhandler for robustness.Suggested approach
Move the call inside the existing
DOMContentLoadedhandler:document.addEventListener("DOMContentLoaded", async () => { + initInstallBanner(); const middleElem = document.querySelector("#middle"); // ... rest of handler }); -initInstallBanner();Or wrap it in its own
DOMContentLoadedcheck:-initInstallBanner(); +if (document.readyState === "loading") { + document.addEventListener("DOMContentLoaded", initInstallBanner); +} else { + initInstallBanner(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 522 - 585, initInstallBanner is invoked at module load and may run before the banner DOM nodes exist; move the invocation so it runs after DOM is ready by either calling initInstallBanner from an existing DOMContentLoaded handler or by checking document.readyState and attaching a document.addEventListener('DOMContentLoaded', ...) fallback; remove the direct initInstallBanner() call at module load and ensure the initInstallBanner function (which uses getElementById, installBtn, dismissBtn, etc.) is only executed after the DOM is parsed.scripts/transform_sw.sh (1)
3-18: Quote shell variables to handle paths with spaces.Variables
$OUTPUT_DIRshould be quoted throughout to prevent word splitting issues with paths containing spaces.Suggested fix
OUTPUT_DIR="$1" COMMIT_HASH=$(git rev-parse --short HEAD) -cp sw.js $OUTPUT_DIR +cp sw.js "$OUTPUT_DIR" # Replace the commit hash in sw.js using sed # (.bak file created for cross-platform compatibility, as some versions of sed require it) -sed -i.bak -r "s/(const COMMIT_HASH = )'[^']*';/\1'$COMMIT_HASH';/" $OUTPUT_DIR/sw.js +sed -i.bak -E "s/(const COMMIT_HASH = )'[^']*';/\1'$COMMIT_HASH';/" "$OUTPUT_DIR/sw.js" if [ $? != 0 ]; then >&2 echo "Failure building sw.js" exit 1 fi -rm $OUTPUT_DIR/sw.js.bak +rm "$OUTPUT_DIR/sw.js.bak"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/transform_sw.sh` around lines 3 - 18, The script uses unquoted $OUTPUT_DIR which breaks on paths with spaces; update usages to quote it (use "$OUTPUT_DIR") for cp, the sed target and the rm command (i.e., cp sw.js "$OUTPUT_DIR", sed ... "$OUTPUT_DIR/sw.js", and rm "$OUTPUT_DIR/sw.js.bak") and keep the existing COMMIT_HASH usage unchanged; this ensures commands operate correctly when OUTPUT_DIR contains spaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.css`:
- Line 725: The CSS uses an undefined variable --bg-color in the rule that sets
background-color: var(--bg-color); replace it with a defined theme variable
(e.g., var(--background-color) to match page background or
var(--dialog-background-color) for a distinct banner) OR add a definition for
--bg-color in :root/theme; update the declaration that contains
background-color: var(--bg-color) accordingly and ensure the chosen variable
exists in the theme.
In `@scripts/transform_sw.sh`:
- Around line 9-11: The sed invocation in scripts/transform_sw.sh uses the
GNU-only -r flag which breaks on macOS; update the sed command in the script
(the line that currently calls sed -i.bak -r "... $OUTPUT_DIR/sw.js") to use -E
instead of -r so the extended-regex option is portable across GNU and BSD/macOS
sed while keeping the -i.bak in-place backup behavior and preserving the
existing regex and target file.
In `@sw.js`:
- Around line 43-54: The cacheFirst function currently calls fetch(request)
without handling network errors; wrap the network call in try/catch inside
cacheFirst so that if fetch throws (offline or failed) you return a safe
fallback Response (e.g., an offline HTML/text response or a new Response('', {
status: 503 })) and avoid calling putInCache when responseFromNetwork is
unavailable; ensure you still call event.waitUntil(putInCache(request,
responseFromNetwork.clone())) only when responseFromNetwork is defined and ok.
- Around line 60-74: The map callback inside the service worker activate handler
(self.addEventListener("activate", ...)) sometimes returns undefined for
whitelist entries; change the code so the callback always returns a Promise by
either replacing cacheNames.map(...) with cacheNames.filter(name =>
!cacheWhitelist.includes(name)).map(name => caches.delete(name)) or by making
the map callback explicitly return a resolved value for whitelist entries (e.g.,
return Promise.resolve() when cacheName is kept). Ensure you update the promise
chain used in event.waitUntil to use the new filtered/map result so all entries
produce a Promise.
- Around line 7-31: PRECACHE_URLS currently contains an external CDN entry which
can cause cache.addAll() in the service worker install handler to fail and abort
installation; update the install flow by removing external origins from
PRECACHE_URLS (or split PRECACHE_URLS into localPrecaches and externalPrecaches
arrays) and then: call caches.open(...) and await cache.addAll(localPrecaches)
inside the install event, and either omit the external CDN from precaching
entirely so it loads via the runtime cache-first handler, or attempt to cache
externalPrecaches in a separate try/catch/promise that tolerates failures
(log/ignore failures) so a fetch error for the CDN does not reject the overall
install; locate symbols PRECACHE_URLS, the install event listener, and
cache.addAll to implement this change.
---
Nitpick comments:
In `@index.css`:
- Around line 741-759: Add visible hover and focus states for
.install-banner-btn and .install-banner-dismiss to improve keyboard
accessibility: implement :hover and :focus (or :focus-visible) rules that change
opacity and add a clear outline (with outline-offset) using a contrasting
variable like --text-color, ensuring the install button and dismiss button both
show distinct visual focus and hover cues while preserving existing padding,
border-radius and colors.
In `@scripts/build.sh`:
- Around line 55-56: The call to scripts/transform_sw.sh uses the unquoted
$OUTPUT_DIR which will break on paths with spaces; update the invocation of
scripts/transform_sw.sh to pass "$OUTPUT_DIR" (i.e., quote the $OUTPUT_DIR
variable) so the script receives the full path correctly and matches quoting
convention used elsewhere in the build script.
In `@scripts/transform_sw.sh`:
- Around line 3-18: The script uses unquoted $OUTPUT_DIR which breaks on paths
with spaces; update usages to quote it (use "$OUTPUT_DIR") for cp, the sed
target and the rm command (i.e., cp sw.js "$OUTPUT_DIR", sed ...
"$OUTPUT_DIR/sw.js", and rm "$OUTPUT_DIR/sw.js.bak") and keep the existing
COMMIT_HASH usage unchanged; this ensures commands operate correctly when
OUTPUT_DIR contains spaces.
In `@src/index.js`:
- Around line 587-599: The service worker is being registered immediately which
can impact first-paint; modify the flow so registerServiceWorker() is invoked on
the window "load" event instead of calling it at script execution. Update the
code that calls registerServiceWorker() so it adds a one-time listener (e.g.,
window.addEventListener("load", ...) or check document.readyState) and then
calls registerServiceWorker(), ensuring registerServiceWorker (and its use of
navigator.serviceWorker.register("/sw.js", { scope: "/" })) remains unchanged
and only runs after the page has fully loaded.
- Around line 522-585: initInstallBanner is invoked at module load and may run
before the banner DOM nodes exist; move the invocation so it runs after DOM is
ready by either calling initInstallBanner from an existing DOMContentLoaded
handler or by checking document.readyState and attaching a
document.addEventListener('DOMContentLoaded', ...) fallback; remove the direct
initInstallBanner() call at module load and ensure the initInstallBanner
function (which uses getElementById, installBtn, dismissBtn, etc.) is only
executed after the DOM is parsed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 904bbb9b-ccf6-4148-ada2-83c379b01b71
📒 Files selected for processing (6)
index.cssindex.htmlscripts/build.shscripts/transform_sw.shsrc/index.jssw.js
Comment on lines
+9
to
+11
| # Replace the commit hash in sw.js using sed | ||
| # (.bak file created for cross-platform compatibility, as some versions of sed require it) | ||
| sed -i.bak -r "s/(const COMMIT_HASH = )'[^']*';/\1'$COMMIT_HASH';/" $OUTPUT_DIR/sw.js |
There was a problem hiding this comment.
sed -r is not portable to macOS.
The -r flag for extended regex is GNU sed specific. macOS uses -E instead. This will cause the build to fail on macOS.
Proposed fix for cross-platform compatibility
# Replace the commit hash in sw.js using sed
# (.bak file created for cross-platform compatibility, as some versions of sed require it)
-sed -i.bak -r "s/(const COMMIT_HASH = )'[^']*';/\1'$COMMIT_HASH';/" $OUTPUT_DIR/sw.js
+sed -i.bak -E "s/(const COMMIT_HASH = )'[^']*';/\1'$COMMIT_HASH';/" "$OUTPUT_DIR/sw.js"The -E flag works on both GNU sed (as an alias for -r) and BSD/macOS sed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Replace the commit hash in sw.js using sed | |
| # (.bak file created for cross-platform compatibility, as some versions of sed require it) | |
| sed -i.bak -r "s/(const COMMIT_HASH = )'[^']*';/\1'$COMMIT_HASH';/" $OUTPUT_DIR/sw.js | |
| # Replace the commit hash in sw.js using sed | |
| # (.bak file created for cross-platform compatibility, as some versions of sed require it) | |
| sed -i.bak -E "s/(const COMMIT_HASH = )'[^']*';/\1'$COMMIT_HASH';/" "$OUTPUT_DIR/sw.js" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/transform_sw.sh` around lines 9 - 11, The sed invocation in
scripts/transform_sw.sh uses the GNU-only -r flag which breaks on macOS; update
the sed command in the script (the line that currently calls sed -i.bak -r "...
$OUTPUT_DIR/sw.js") to use -E instead of -r so the extended-regex option is
portable across GNU and BSD/macOS sed while keeping the -i.bak in-place backup
behavior and preserving the existing regex and target file.
Comment on lines
+7
to
+31
| const PRECACHE_URLS = [ | ||
| "/", | ||
| "/index.html", | ||
| "/CHANGELOG.html", | ||
| "/config.json", | ||
| "/images/rotate-device.png", | ||
| "/vendor/dialog-polyfill.css", | ||
| "/words.txt", | ||
| "/index.css", | ||
| "/src/index.js", | ||
| "/src/share/index.js", | ||
| "/src/share/browser.js", | ||
| "/src/storage/index.js", | ||
| "/src/storage/browser.js", | ||
| "/src/consts.js", | ||
| "/src/datetime.js", | ||
| "/src/dialogManager.js", | ||
| "/src/game.js", | ||
| "/src/preferences.js", | ||
| "/src/render.js", | ||
| "/vendor/snow.js", | ||
| "/vendor/dialog-polyfill.js", | ||
| "/vendor/feather.min.js", | ||
| "https://cdnjs.cloudflare.com/ajax/libs/mobile-detect/1.4.5/mobile-detect.min.js", | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if mobile-detect is also loaded from a local vendor copy
rg -n "mobile-detect" --glob "*.html" --glob "*.js"Repository: Coteh/wordle-clone
Length of output: 283
🏁 Script executed:
cat -n sw.jsRepository: Coteh/wordle-clone
Length of output: 3124
🌐 Web query:
cdnjs.cloudflare.com CORS headers mobile-detect.min.js
💡 Result:
For the cdnjs-hosted file:
- URL:
https://cdnjs.cloudflare.com/ajax/libs/mobile-detect/1.4.5/mobile-detect.min.js(shown on the official cdnjs library page) [1] - CORS behavior: cdnjs assets are served with an
Access-Control-Allow-Origin: *response header (sofetch()/XHR from any origin is allowed as long as you’re not sending credentials). A captured response header example from cdnjs showsaccess-control-allow-origin: *[2]. (CORS is controlled by response headers likeAccess-Control-Allow-Origin.) [3]
Verify from your device/network (recommended):
curl -I https://cdnjs.cloudflare.com/ajax/libs/mobile-detect/1.4.5/mobile-detect.min.jsYou should see access-control-allow-origin: *.
Common gotcha (especially on mobile webviews):
- If you do
fetch(url, { credentials: "include" }), it will fail becauseAccess-Control-Allow-Origin: *cannot be used with credentials. Usecredentials: "omit"(default) or host/proxy the file yourself with a specific origin andAccess-Control-Allow-Credentials: true.
If you tell me whether you’re loading it via a <script> tag, ES module (type="module"), or fetch(), plus the exact console error text, I can pinpoint the fix.
Sources: [1] [2] [3]
External CDN dependency in precache may block service worker installation.
The cdnjs URL (line 30) lacks error handling. If cache.addAll() fails to fetch any resource (CDN unreachable, timeout, network error), the entire install event fails and the service worker won't install. While cdnjs has proper CORS headers, the availability risk remains.
Consider:
- Handling cross-origin resources separately with error tolerance (split into local and remote batches with separate error handling), or
- Removing external CDN resources from precache and letting them cache on-demand via the
cacheFirsthandler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sw.js` around lines 7 - 31, PRECACHE_URLS currently contains an external CDN
entry which can cause cache.addAll() in the service worker install handler to
fail and abort installation; update the install flow by removing external
origins from PRECACHE_URLS (or split PRECACHE_URLS into localPrecaches and
externalPrecaches arrays) and then: call caches.open(...) and await
cache.addAll(localPrecaches) inside the install event, and either omit the
external CDN from precaching entirely so it loads via the runtime cache-first
handler, or attempt to cache externalPrecaches in a separate try/catch/promise
that tolerates failures (log/ignore failures) so a fetch error for the CDN does
not reject the overall install; locate symbols PRECACHE_URLS, the install event
listener, and cache.addAll to implement this change.
Comment on lines
+43
to
+54
| const cacheFirst = async (request, event) => { | ||
| const cache = await caches.open(CACHE_NAME); | ||
| const responseFromCache = await cache.match(request); | ||
| if (responseFromCache) { | ||
| if (responseFromCache.ok) { | ||
| return responseFromCache; | ||
| } | ||
| } | ||
| const responseFromNetwork = await fetch(request); | ||
| event.waitUntil(putInCache(request, responseFromNetwork.clone())); | ||
| return responseFromNetwork; | ||
| }; |
There was a problem hiding this comment.
Add error handling for network failures.
When both cache miss and network request fail (e.g., user is offline on first visit), fetch(request) throws an unhandled error. Consider wrapping in try/catch with a fallback response.
Proposed fix
const cacheFirst = async (request, event) => {
const cache = await caches.open(CACHE_NAME);
const responseFromCache = await cache.match(request);
if (responseFromCache) {
if (responseFromCache.ok) {
return responseFromCache;
}
}
- const responseFromNetwork = await fetch(request);
- event.waitUntil(putInCache(request, responseFromNetwork.clone()));
- return responseFromNetwork;
+ try {
+ const responseFromNetwork = await fetch(request);
+ event.waitUntil(putInCache(request, responseFromNetwork.clone()));
+ return responseFromNetwork;
+ } catch (error) {
+ // Network failed and no cache available
+ return new Response("Network error", {
+ status: 408,
+ headers: { "Content-Type": "text/plain" },
+ });
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const cacheFirst = async (request, event) => { | |
| const cache = await caches.open(CACHE_NAME); | |
| const responseFromCache = await cache.match(request); | |
| if (responseFromCache) { | |
| if (responseFromCache.ok) { | |
| return responseFromCache; | |
| } | |
| } | |
| const responseFromNetwork = await fetch(request); | |
| event.waitUntil(putInCache(request, responseFromNetwork.clone())); | |
| return responseFromNetwork; | |
| }; | |
| const cacheFirst = async (request, event) => { | |
| const cache = await caches.open(CACHE_NAME); | |
| const responseFromCache = await cache.match(request); | |
| if (responseFromCache) { | |
| if (responseFromCache.ok) { | |
| return responseFromCache; | |
| } | |
| } | |
| try { | |
| const responseFromNetwork = await fetch(request); | |
| event.waitUntil(putInCache(request, responseFromNetwork.clone())); | |
| return responseFromNetwork; | |
| } catch (error) { | |
| // Network failed and no cache available | |
| return new Response("Network error", { | |
| status: 408, | |
| headers: { "Content-Type": "text/plain" }, | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sw.js` around lines 43 - 54, The cacheFirst function currently calls
fetch(request) without handling network errors; wrap the network call in
try/catch inside cacheFirst so that if fetch throws (offline or failed) you
return a safe fallback Response (e.g., an offline HTML/text response or a new
Response('', { status: 503 })) and avoid calling putInCache when
responseFromNetwork is unavailable; ensure you still call
event.waitUntil(putInCache(request, responseFromNetwork.clone())) only when
responseFromNetwork is defined and ok.
Comment on lines
+60
to
+74
| self.addEventListener("activate", (event) => { | ||
| const cacheWhitelist = [CACHE_NAME]; | ||
| event.waitUntil( | ||
| caches.keys().then(cacheNames => | ||
| Promise.all( | ||
| cacheNames.map(cacheName => { | ||
| if (!cacheWhitelist.includes(cacheName)) { | ||
| // This cache does not match the current version, delete | ||
| return caches.delete(cacheName); | ||
| } | ||
| }) | ||
| ) | ||
| ) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix map callback to always return a value.
The map callback doesn't return a value when cacheName is in the whitelist. Use filter + map or add explicit return.
Proposed fix using filter
self.addEventListener("activate", (event) => {
const cacheWhitelist = [CACHE_NAME];
event.waitUntil(
caches.keys().then(cacheNames =>
Promise.all(
- cacheNames.map(cacheName => {
- if (!cacheWhitelist.includes(cacheName)) {
- // This cache does not match the current version, delete
- return caches.delete(cacheName);
- }
- })
+ cacheNames
+ .filter(cacheName => !cacheWhitelist.includes(cacheName))
+ .map(cacheName => caches.delete(cacheName))
)
)
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.addEventListener("activate", (event) => { | |
| const cacheWhitelist = [CACHE_NAME]; | |
| event.waitUntil( | |
| caches.keys().then(cacheNames => | |
| Promise.all( | |
| cacheNames.map(cacheName => { | |
| if (!cacheWhitelist.includes(cacheName)) { | |
| // This cache does not match the current version, delete | |
| return caches.delete(cacheName); | |
| } | |
| }) | |
| ) | |
| ) | |
| ); | |
| }); | |
| self.addEventListener("activate", (event) => { | |
| const cacheWhitelist = [CACHE_NAME]; | |
| event.waitUntil( | |
| caches.keys().then(cacheNames => | |
| Promise.all( | |
| cacheNames | |
| .filter(cacheName => !cacheWhitelist.includes(cacheName)) | |
| .map(cacheName => caches.delete(cacheName)) | |
| ) | |
| ) | |
| ); | |
| }); |
🧰 Tools
🪛 Biome (2.4.6)
[error] 65-65: This callback passed to map() iterable method should always return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sw.js` around lines 60 - 74, The map callback inside the service worker
activate handler (self.addEventListener("activate", ...)) sometimes returns
undefined for whitelist entries; change the code so the callback always returns
a Promise by either replacing cacheNames.map(...) with cacheNames.filter(name =>
!cacheWhitelist.includes(name)).map(name => caches.delete(name)) or by making
the map callback explicitly return a resolved value for whitelist entries (e.g.,
return Promise.resolve() when cacheName is kept). Ensure you update the promise
chain used in event.waitUntil to use the new filtered/map result so all entries
produce a Promise.
Test Results 16 files + 2 111 suites +24 8m 21s ⏱️ + 3m 19s For more details on these failures, see this check. Results for commit 52586e5. ± Comparison against base commit d0a04d6. This pull request removes 1 and adds 43 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
- Fix transparent background by using --fallback-background-color (--bg-color was never defined as a CSS variable) - On iOS, hide the generic banner text and give the iOS instruction flex:1 so it's the only text shown and has room to display properly https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Clicking anywhere outside the banner hides it for the current session but does not write to localStorage, so it reappears on next page load. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Deploying wordle-clone with
|
| Latest commit: |
52586e5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://497d60ee.wordle-clone-8k1.pages.dev |
| Branch Preview URL: | https://claude-add-pwa-install-promp.wordle-clone-8k1.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/index.js (2)
532-533: Consider adding null checks for button elements.If the HTML structure is malformed or elements are missing, accessing
installBtnordismissBtnproperties (lines 551, 564, 580) will throw aTypeError. Adding guards would make this more resilient.Proposed defensive check
const installBtn = document.getElementById("install-btn"); const dismissBtn = document.getElementById("install-dismiss"); + if (!installBtn || !dismissBtn) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 532 - 533, The code assumes DOM elements exist for installBtn and dismissBtn (variables installBtn and dismissBtn) and will throw if they are null; update the logic to guard all usages of installBtn and dismissBtn (where you add event listeners or access properties) by checking for truthiness first (e.g., if (!installBtn) return/skip or wrap addEventListener calls in if checks) so missing elements are handled gracefully and no TypeError is thrown at runtime.
580-586: Dismissal persists even if user cancels the install prompt.After
userChoiceresolves,hideBanner(true)is called unconditionally, storing the dismissal in localStorage. If the user clicked "Cancel" on the native prompt, they won't see the banner on future visits. If this is intentional (user actively engaged), the current behavior is fine. Otherwise, consider checking the outcome:Optional: Only persist dismissal on actual install
installBtn.addEventListener("click", async () => { if (!deferredPrompt) return; deferredPrompt.prompt(); - await deferredPrompt.userChoice; + const { outcome } = await deferredPrompt.userChoice; deferredPrompt = null; - hideBanner(true); + hideBanner(outcome === "accepted"); });The
appinstalledevent at line 588 will still persist dismissal on successful install.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 580 - 586, The install button handler currently calls hideBanner(true) unconditionally after awaiting deferredPrompt.userChoice, which stores a persistent dismissal even when the user cancels; change the handler in the installBtn click listener to inspect the resolved value from deferredPrompt.userChoice (e.g., let choice = await deferredPrompt.userChoice) and only call hideBanner(true) (and clear deferredPrompt) when choice.outcome === 'accepted' (leave the banner shown or do a non-persistent hide if outcome === 'dismissed' or 'cancelled'); keep the existing appinstalled event behavior that persists dismissal on successful installs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.js`:
- Line 592: initInstallBanner() is being called at module load and can run
before the DOM exists; move the invocation into the DOM-ready path or add a
readiness guard so it runs only after parsing. Modify the code to either call
initInstallBanner() from the existing DOMContentLoaded handler (the one that
currently initializes other UI logic) or wrap the call with a check like if
(document.readyState === "loading") {
document.addEventListener("DOMContentLoaded", initInstallBanner); } else {
initInstallBanner(); } so the function (initInstallBanner) always runs when DOM
elements accessed via getElementById are present.
---
Nitpick comments:
In `@src/index.js`:
- Around line 532-533: The code assumes DOM elements exist for installBtn and
dismissBtn (variables installBtn and dismissBtn) and will throw if they are
null; update the logic to guard all usages of installBtn and dismissBtn (where
you add event listeners or access properties) by checking for truthiness first
(e.g., if (!installBtn) return/skip or wrap addEventListener calls in if checks)
so missing elements are handled gracefully and no TypeError is thrown at
runtime.
- Around line 580-586: The install button handler currently calls
hideBanner(true) unconditionally after awaiting deferredPrompt.userChoice, which
stores a persistent dismissal even when the user cancels; change the handler in
the installBtn click listener to inspect the resolved value from
deferredPrompt.userChoice (e.g., let choice = await deferredPrompt.userChoice)
and only call hideBanner(true) (and clear deferredPrompt) when choice.outcome
=== 'accepted' (leave the banner shown or do a non-persistent hide if outcome
=== 'dismissed' or 'cancelled'); keep the existing appinstalled event behavior
that persists dismissal on successful installs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b445d80-9e5e-4fad-a595-09aef31c6e98
📒 Files selected for processing (2)
index.csssrc/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- index.css
| } | ||
| }; | ||
|
|
||
| initInstallBanner(); |
There was a problem hiding this comment.
Move initInstallBanner() inside DOMContentLoaded or guard with DOM readiness check.
This function is invoked at module level, but it accesses DOM elements via getElementById. If the script executes before the DOM is fully parsed (e.g., script in <head> without defer), the banner element won't exist, the function returns early, and the install banner never initializes—even after the DOM becomes ready.
Proposed fix: Wrap in DOMContentLoaded check
-initInstallBanner();
+if (document.readyState === "loading") {
+ document.addEventListener("DOMContentLoaded", initInstallBanner);
+} else {
+ initInstallBanner();
+}Alternatively, move the call inside the existing DOMContentLoaded handler at line 53.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| initInstallBanner(); | |
| if (document.readyState === "loading") { | |
| document.addEventListener("DOMContentLoaded", initInstallBanner); | |
| } else { | |
| initInstallBanner(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.js` at line 592, initInstallBanner() is being called at module load
and can run before the DOM exists; move the invocation into the DOM-ready path
or add a readiness guard so it runs only after parsing. Modify the code to
either call initInstallBanner() from the existing DOMContentLoaded handler (the
one that currently initializes other UI logic) or wrap the call with a check
like if (document.readyState === "loading") {
document.addEventListener("DOMContentLoaded", initInstallBanner); } else {
initInstallBanner(); } so the function (initInstallBanner) always runs when DOM
elements accessed via getElementById are present.
Tests cover: - Banner not visible before beforeinstallprompt fires - Banner appears after event fires and 1.5s delay - Permanent dismiss via X button (hides banner, sets localStorage, no reappear on reload) - Temporary close via click outside (hides banner, no localStorage, reappears on reload) - Already dismissed: banner never shows even after event fires - Standalone mode: banner never shows - iOS: auto-appears after delay, shows share instruction, hides install button, permanent dismiss and temporary close both work correctly https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/e2e/game/install-banner.cy.js`:
- Around line 34-38: The onBeforeLoad callback passed to cy.visit currently uses
the global window instead of the callback parameter; change the callback to
accept the parameter (e.g., win) and call
win.localStorage.setItem("wc_played_before", true) so it matches other
onBeforeLoad usages in this file (see the cy.visit onBeforeLoad callbacks at
lines with win parameter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 312d8b93-3666-41af-a419-cec2888839f1
📒 Files selected for processing (1)
cypress/e2e/game/install-banner.cy.js
Comment on lines
+34
to
+38
| cy.visit("/", { | ||
| onBeforeLoad: () => { | ||
| window.localStorage.setItem("wc_played_before", true); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Inconsistent window reference in onBeforeLoad callback.
This callback uses the global window object instead of the application window passed as a parameter. This is inconsistent with other onBeforeLoad callbacks in this file (lines 127, 142, 165) which correctly use the win parameter.
Proposed fix for consistency
cy.visit("/", {
- onBeforeLoad: () => {
- window.localStorage.setItem("wc_played_before", true);
+ onBeforeLoad: (win) => {
+ win.localStorage.setItem("wc_played_before", true);
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cy.visit("/", { | |
| onBeforeLoad: () => { | |
| window.localStorage.setItem("wc_played_before", true); | |
| }, | |
| }); | |
| cy.visit("/", { | |
| onBeforeLoad: (win) => { | |
| win.localStorage.setItem("wc_played_before", true); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/game/install-banner.cy.js` around lines 34 - 38, The onBeforeLoad
callback passed to cy.visit currently uses the global window instead of the
callback parameter; change the callback to accept the parameter (e.g., win) and
call win.localStorage.setItem("wc_played_before", true) so it matches other
onBeforeLoad usages in this file (see the cy.visit onBeforeLoad callbacks at
lines with win parameter).
Colours: - Use --dialog-background-color and --text-color so the banner respects all themes (dark, light, snow) instead of being solid black How to Play coexistence: - If the initial HTP dialog is open when the banner wants to appear, defer via MutationObserver and show after a 2s grace period once HTP closes - If HTP is re-opened manually (help icon) after the initial close, the banner shows normally — both can be visible simultaneously - When the banner is visible, clicks on the overlay-back no longer close the dialog behind it; only the banner is dismissed - Outside-click handler is now guarded so it only fires when the banner is actually visible iOS: - Replace Unicode arrow with feather share-2 icon for a more accurate representation of the iOS share sheet button https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.js (1)
618-626: Don't permanently suppress the banner after a canceled native prompt.
hideBanner(true)runs for both accepted and dismisseduserChoiceoutcomes. If the dismissal flag should only persist after an explicit banner dismiss or successful install, branch on theoutcomeproperty and reserve the suppression for accepted installs.💡 Suggested fix
installBtn.addEventListener("click", async () => { if (!deferredPrompt) return; deferredPrompt.prompt(); - await deferredPrompt.userChoice; + const { outcome } = await deferredPrompt.userChoice; deferredPrompt = null; - hideBanner(true); + hideBanner(outcome === "accepted"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 618 - 626, The install button handler and the deferredPrompt flow currently call hideBanner(true) unconditionally, which hides the banner even when the user dismisses the native install prompt; update the installBtn click listener to await deferredPrompt.userChoice, inspect the returned object's outcome property (e.g., choice.outcome) and only call hideBanner(true) when outcome === "accepted" (keep calling hideBanner(true) in the window "appinstalled" listener and call hideBanner(false) or leave the banner visible for dismissed outcomes), so that the banner suppression persists only after an explicit dismiss or a successful install; reference the installBtn click handler, deferredPrompt, and hideBanner functions when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.js`:
- Around line 592-595: The current isIOS detection (variable isIOS) misses
modern iPadOS devices that report macOS-like user agents; update the isIOS
assignment to also detect iPadOS by checking for desktop-class platform with
touch support—for example, keep the existing
/iphone|ipad|ipod/i.test(navigator.userAgent) && !window.MSStream check and OR
it with a condition like (navigator.platform === 'MacIntel' &&
navigator.maxTouchPoints > 1) so iPadOS Safari is treated as iOS and will not
fall into the beforeinstallprompt branch.
---
Nitpick comments:
In `@src/index.js`:
- Around line 618-626: The install button handler and the deferredPrompt flow
currently call hideBanner(true) unconditionally, which hides the banner even
when the user dismisses the native install prompt; update the installBtn click
listener to await deferredPrompt.userChoice, inspect the returned object's
outcome property (e.g., choice.outcome) and only call hideBanner(true) when
outcome === "accepted" (keep calling hideBanner(true) in the window
"appinstalled" listener and call hideBanner(false) or leave the banner visible
for dismissed outcomes), so that the banner suppression persists only after an
explicit dismiss or a successful install; reference the installBtn click
handler, deferredPrompt, and hideBanner functions when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 031fb71f-02a8-49e7-9f09-b8b52fd87a2c
📒 Files selected for processing (2)
index.csssrc/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- index.css
Comment on lines
+592
to
+595
| // iOS detection | ||
| const isIOS = | ||
| /iphone|ipad|ipod/i.test(navigator.userAgent) && !window.MSStream; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On current iPadOS/Safari versions, do iPads always include "iPad" in navigator.userAgent, and does Safari expose beforeinstallprompt? What JavaScript detection pattern is recommended for identifying iPadOS devices in web apps?
💡 Result:
- No—on current iPadOS/Safari, you cannot assume
navigator.userAgentalways contains"iPad". Since iPadOS 13, Safari on iPad can send a macOS-like UA (often containing"Macintosh"), so UA-sniffing for"iPad"is unreliable. [1] Some configurations/situations still yield an"iPad; CPU OS …"UA, so you may see either form. [2] - No—Safari (iPadOS/iOS) does not expose the Chromium-style
beforeinstallpromptevent, so it won’t fire there. [3][4]
Recommended iPadOS detection pattern (web apps): prefer feature detection; if you specifically need to identify iPadOS devices, use the widely used “MacIntel + touch points” heuristic:
const isIPadOS =
navigator.platform === "MacIntel" &&
navigator.maxTouchPoints > 1; // (iPad masquerading as Mac)This targets iPads reporting as “Mac” while excluding most Macs (which typically have maxTouchPoints === 0). [1][5]
Sources: WebKit Safari 13 iPad UA change [1]; iPad UA examples/history [2]; beforeinstallprompt support tables [3]; MDN guidance noting you must check cross-browser support (Safari lacks it) [4]; iPadOS detection heuristic example [5].
[1] (webkit.org) [2] (nielsleenheer.com) [3] (caniuse.com) [4] (developer.mozilla.org) [5] (doc.dynamicweb.dev)
Citations:
- 1: https://webkit.org/blog/9674/new-webkit-features-in-safari-13/?utm_source=openai
- 2: https://nielsleenheer.com/articles/2025/the-user-agent-string-of-safari-on-ios-26-and-macos-26/?utm_source=openai
- 3: https://caniuse.com/mdn-api_window_beforeinstallprompt_event?utm_source=openai
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeinstallprompt_event?utm_source=openai
- 5: https://doc.dynamicweb.dev/documentation/fundamentals/faq/device-detection.html?utm_source=openai
🏁 Script executed:
sed -n '590,620p' src/index.jsRepository: Coteh/wordle-clone
Length of output: 1247
Broaden the iOS check to catch desktop-class iPadOS.
Modern iPads (iOS 13+) can report macOS-like user agents without "iPad" in the string, bypassing the iphone|ipad|ipod regex and falling into the beforeinstallprompt branch. Since Safari on iPadOS does not support the beforeinstallprompt event, these users never see install guidance.
Suggested fix
- const isIOS =
- /iphone|ipad|ipod/i.test(navigator.userAgent) && !window.MSStream;
+ const isIOS =
+ (
+ /iphone|ipad|ipod/i.test(navigator.userAgent) ||
+ (navigator.platform === "MacIntel" && navigator.maxTouchPoints > 1)
+ ) && !window.MSStream;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.js` around lines 592 - 595, The current isIOS detection (variable
isIOS) misses modern iPadOS devices that report macOS-like user agents; update
the isIOS assignment to also detect iPadOS by checking for desktop-class
platform with touch support—for example, keep the existing
/iphone|ipad|ipod/i.test(navigator.userAgent) && !window.MSStream check and OR
it with a condition like (navigator.platform === 'MacIntel' &&
navigator.maxTouchPoints > 1) so iPadOS Safari is treated as iOS and will not
fall into the beforeinstallprompt branch.
Lucide is an actively maintained fork of Feather with a larger icon set, including a proper platform-appropriate share icon (lucide:share) used for the iOS install banner instruction instead of the generic share-2. Changes: - Replace vendor/feather.min.js with vendor/lucide.min.js (v0.577.0) - Replace all data-feather attributes with data-lucide equivalents - Replace feather.replace() calls with lucide.createIcons() - iOS banner: data-lucide="share" (was data-feather="share-2") - Update sw.js precache list to reference lucide.min.js - Update Cypress commands.js selector: .feather.feather-delete -> .lucide.lucide-delete - Update misc.cy.js fallback icon test to intercept lucide.min.js https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Minor lockfile cleanup from npm removing a redundant "peer" field on the enquirer entries during the lucide npm install/uninstall cycle. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/index.js (1)
592-595:⚠️ Potential issue | 🟠 MajorBroaden iOS detection to include desktop-class iPadOS.
At Line 593, UA-only detection misses iPadOS devices reporting as Mac, which can skip the iOS install-instructions path.
Suggested fix
- const isIOS = - /iphone|ipad|ipod/i.test(navigator.userAgent) && !window.MSStream; + const isIOS = + ( + /iphone|ipad|ipod/i.test(navigator.userAgent) || + (navigator.platform === "MacIntel" && navigator.maxTouchPoints > 1) + ) && !window.MSStream;On current iPadOS/Safari versions, is `navigator.userAgent` matching `/iphone|ipad|ipod/` sufficient for iPad detection, or should web apps also use `navigator.platform === "MacIntel" && navigator.maxTouchPoints > 1`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 592 - 595, The current isIOS detection (const isIOS) only checks navigator.userAgent for iphone|ipad|ipod and misses iPadOS devices that report as Mac; update the isIOS logic to also treat devices with navigator.platform === "MacIntel" && navigator.maxTouchPoints > 1 as iOS/iPadOS so the install-instructions path runs on desktop-class iPads — modify the isIOS constant in src/index.js to include that additional condition alongside the existing UA regex.sw.js (3)
65-70:⚠️ Potential issue | 🟡 MinorFix
mapcallback to consistently return values in activate cleanup.Line 65 uses
mapwith a branch that returns nothing, which matches the Biome error.Suggested fix
- cacheNames.map(cacheName => { - if (!cacheWhitelist.includes(cacheName)) { - // This cache does not match the current version, delete - return caches.delete(cacheName); - } - }) + cacheNames + .filter((cacheName) => !cacheWhitelist.includes(cacheName)) + .map((cacheName) => caches.delete(cacheName))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sw.js` around lines 65 - 70, The activate handler currently uses cacheNames.map with a branch that sometimes returns undefined; update the callback so it always returns a Promise (or use filter+map) — e.g., replace cacheNames.map(...) with cacheNames.filter(name => !cacheWhitelist.includes(name)).map(name => caches.delete(name)) or ensure the map callback returns caches.delete(cacheName) for non-whitelisted entries and a resolved value for others, then await Promise.all(...) to perform the deletions; key symbols: cacheNames, cacheWhitelist, caches.delete, and the map callback in the activate cleanup.
7-31:⚠️ Potential issue | 🟠 MajorExternal CDN in precache can still block service worker install.
At Line 30, the CDN URL is in
PRECACHE_URLS. Withcache.addAll()(Line 57), one fetch failure rejects the whole install.Suggested fix
-const PRECACHE_URLS = [ +const PRECACHE_URLS = [ "/", "/index.html", @@ "/vendor/dialog-polyfill.js", "/vendor/lucide.min.js", - "https://cdnjs.cloudflare.com/ajax/libs/mobile-detect/1.4.5/mobile-detect.min.js", ]; + +const OPTIONAL_EXTERNAL_PRECACHE_URLS = [ + "https://cdnjs.cloudflare.com/ajax/libs/mobile-detect/1.4.5/mobile-detect.min.js", +]; @@ self.addEventListener("install", (event) => { - event.waitUntil(addResourcesToCache(PRECACHE_URLS)); + event.waitUntil((async () => { + await addResourcesToCache(PRECACHE_URLS); + try { + await addResourcesToCache(OPTIONAL_EXTERNAL_PRECACHE_URLS); + } catch (e) { + console.warn("Optional external precache failed:", e); + } + })()); });#!/bin/bash rg -n -C2 'PRECACHE_URLS|cdnjs.cloudflare.com|addAll|addResourcesToCache' sw.jsAlso applies to: 56-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sw.js` around lines 7 - 31, PRECACHE_URLS contains an external CDN entry which will make cache.addAll (used in the install flow) fail the whole SW install if that fetch fails; remove the CDN URL from PRECACHE_URLS and instead fetch and cache that resource during runtime or inside a guarded helper (e.g., in addResourcesToCache or a new fetchAndCache function) using individual cache.add/fetch wrapped in try/catch so failures are ignored and do not reject the install; update code references PRECACHE_URLS and the install logic that calls cache.addAll/addResourcesToCache accordingly.
43-54:⚠️ Potential issue | 🟠 Major
cacheFirst()still has offline and request-type handling gaps.At Lines 43-54 and Lines 76-78:
- network failure is unhandled,
- cached opaque responses are ignored due to
responseFromCache.ok,- all request methods are intercepted/cached.
Suggested fix
const cacheFirst = async (request, event) => { const cache = await caches.open(CACHE_NAME); const responseFromCache = await cache.match(request); - if (responseFromCache) { - if (responseFromCache.ok) { - return responseFromCache; - } - } - const responseFromNetwork = await fetch(request); - event.waitUntil(putInCache(request, responseFromNetwork.clone())); - return responseFromNetwork; + if (responseFromCache) return responseFromCache; + + try { + const responseFromNetwork = await fetch(request); + if ( + request.method === "GET" && + (responseFromNetwork.ok || responseFromNetwork.type === "opaque") + ) { + event.waitUntil(putInCache(request, responseFromNetwork.clone())); + } + return responseFromNetwork; + } catch (error) { + return new Response("Offline", { + status: 503, + headers: { "Content-Type": "text/plain" }, + }); + } }; @@ self.addEventListener("fetch", (event) => { + if (event.request.method !== "GET") return; event.respondWith(cacheFirst(event.request, event)); });#!/bin/bash rg -n -C3 'cacheFirst|responseFromCache|responseFromNetwork|request.method|addEventListener\("fetch"' sw.jsAlso applies to: 76-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sw.js` around lines 43 - 54, The cacheFirst function currently ignores opaque cached responses, doesn't handle fetch failures, and caches all request methods; update cacheFirst and the fetch handler to: 1) only handle and putInCache for GET requests by checking request.method === 'GET' before intercepting or caching, 2) treat a cached response as valid if it exists even when responseFromCache.ok is false by removing/relaxing the responseFromCache.ok check (or explicitly allow responseFromCache.type === 'opaque'), 3) wrap the network fetch in try/catch so that on network errors you return the cached response if present (or a fallback) and avoid unhandled rejections, and 4) ensure you clone the network response before calling putInCache(response.clone()) and call event.waitUntil only when caching is actually scheduled; reference cacheFirst, responseFromCache, responseFromNetwork, putInCache, CACHE_NAME, and the fetch event handler/request.method to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/index.js`:
- Around line 592-595: The current isIOS detection (const isIOS) only checks
navigator.userAgent for iphone|ipad|ipod and misses iPadOS devices that report
as Mac; update the isIOS logic to also treat devices with navigator.platform ===
"MacIntel" && navigator.maxTouchPoints > 1 as iOS/iPadOS so the
install-instructions path runs on desktop-class iPads — modify the isIOS
constant in src/index.js to include that additional condition alongside the
existing UA regex.
In `@sw.js`:
- Around line 65-70: The activate handler currently uses cacheNames.map with a
branch that sometimes returns undefined; update the callback so it always
returns a Promise (or use filter+map) — e.g., replace cacheNames.map(...) with
cacheNames.filter(name => !cacheWhitelist.includes(name)).map(name =>
caches.delete(name)) or ensure the map callback returns caches.delete(cacheName)
for non-whitelisted entries and a resolved value for others, then await
Promise.all(...) to perform the deletions; key symbols: cacheNames,
cacheWhitelist, caches.delete, and the map callback in the activate cleanup.
- Around line 7-31: PRECACHE_URLS contains an external CDN entry which will make
cache.addAll (used in the install flow) fail the whole SW install if that fetch
fails; remove the CDN URL from PRECACHE_URLS and instead fetch and cache that
resource during runtime or inside a guarded helper (e.g., in addResourcesToCache
or a new fetchAndCache function) using individual cache.add/fetch wrapped in
try/catch so failures are ignored and do not reject the install; update code
references PRECACHE_URLS and the install logic that calls
cache.addAll/addResourcesToCache accordingly.
- Around line 43-54: The cacheFirst function currently ignores opaque cached
responses, doesn't handle fetch failures, and caches all request methods; update
cacheFirst and the fetch handler to: 1) only handle and putInCache for GET
requests by checking request.method === 'GET' before intercepting or caching, 2)
treat a cached response as valid if it exists even when responseFromCache.ok is
false by removing/relaxing the responseFromCache.ok check (or explicitly allow
responseFromCache.type === 'opaque'), 3) wrap the network fetch in try/catch so
that on network errors you return the cached response if present (or a fallback)
and avoid unhandled rejections, and 4) ensure you clone the network response
before calling putInCache(response.clone()) and call event.waitUntil only when
caching is actually scheduled; reference cacheFirst, responseFromCache,
responseFromNetwork, putInCache, CACHE_NAME, and the fetch event
handler/request.method to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2058e324-1f74-4700-80a8-9ef9bda1a0ed
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonvendor/feather.min.jsis excluded by!**/*.min.jsvendor/lucide.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (6)
cypress/e2e/game/misc.cy.jscypress/support/commands.jsindex.htmlsrc/index.jssrc/render.jssw.js
"To install, tap [share] → Add to Home Screen" makes the install intent explicit upfront while staying concise. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Keyboard keys call e.preventDefault() on touchstart, which suppresses the synthetic click event on mobile. The banner's outside-interaction handler now also listens for touchstart on the document so key taps correctly close the banner. The existing click listener is retained for mouse/desktop interactions. The visible-class guard prevents double- firing on devices that would generate both events. Adds Cypress tests verifying that a touchstart on a keyboard key hides the banner temporarily without writing to localStorage. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
A transparent fixed overlay at z-index 199 (just below the banner at 200) covers the game while the banner is shown. Touches on the keyboard or game board hit the overlay instead, so no input is registered. The overlay is removed after the banner's hide animation completes (350ms), matching the banner's own transition timing. Adds Cypress tests for: - Key tap not registering as game input - Overlay visible while banner is visible - Overlay removed once banner is hidden https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
…verlay Without e.preventDefault() on the overlay's touchstart, the browser still synthesizes mousedown/mouseup events after touchend. These can fire at the tap location once the overlay clears (~350ms), hitting the keyboard key underneath and registering unintended input. Calling e.preventDefault() on the overlay's touchstart suppresses all synthetic mouse events from that touch sequence. The touchstart still bubbles to the document-level handler which closes the banner normally. Also fixes the Cypress tests: the "should not register key tap as game input" test was triggering directly on the keyboard item, bypassing the overlay entirely. Tests now trigger on the overlay (matching real-device behaviour) and verify both that no input is registered and that the banner closes. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Replace "Install" button label and tweak the description to make the action more approachable — "Add to Home Screen" is familiar phrasing users already see in their browser menus, while "Install" can feel more intrusive. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
- Revert Install button text back to "Install" (Add to Home Screen was
too broad; iOS already shows its own tailored message)
- Add position option ("top" | "bottom", default "bottom"): "top" slides
the banner in from the top via a new .install-banner--top CSS modifier
that flips the anchor, border, shadow, and slide-in transform
- Add dismissOnOutsideInteraction option (default true): when false the
overlay is never shown and no outside-click/touchstart listeners are
registered, letting the player keep playing with the banner open
- Expose both options explicitly at the call site with inline comments so
they are easy to find and flip for other games
https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Previously false meant "never dismiss on outside interaction". Now: - true (default): overlay covers the page; any outside touch/click dismisses the banner and the overlay prevents the element underneath from also activating (existing behaviour, unchanged) - false: no overlay; outside interactions still dismiss the banner UNLESS the touched element matches a selector in interactionWhitelist, so the player can keep typing on the keyboard while the banner is open The call site now shows the whitelist option commented out as a ready- to-use example for switching to the non-overlay mode. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
…T-116) Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
…s migration tests Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
…, remove stale test, combine Cypress tests Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
…rage key Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
…ting index.html or changelog
This is to make it more clear this is not a generic dialog with OK button but actually the dialog that is shown when data is migrated.
…LOCAL* environments Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
…ck in error handling test Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
… constant; simplify build.sh transform call Co-authored-by: Coteh <3276350+Coteh@users.noreply.github.com>
Lucide is an actively maintained fork of Feather with a larger icon set, including a proper platform-appropriate share icon (lucide:share) used for the iOS install banner instruction instead of the generic share-2. Changes: - Replace vendor/feather.min.js with vendor/lucide.min.js (v0.577.0) - Replace all data-feather attributes with data-lucide equivalents - Replace feather.replace() calls with lucide.createIcons() - iOS banner: data-lucide="share" (was data-feather="share-2") - Update sw.js precache list to reference lucide.min.js - Update Cypress commands.js selector: .feather.feather-delete -> .lucide.lucide-delete - Update misc.cy.js fallback icon test to intercept lucide.min.js https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Migration dialog coexistence (task 1):
- showBanner now detects an open .dialog and always raises the install-
banner overlay when one is present, regardless of dismissOnOutsideIn-
teraction. The overlay (z-index 199) sits above the dialog (z-index
101), so outside taps hit the overlay and dismiss the banner without
reaching the dialog. Once the banner is gone the dialog is interactive.
- hideBanner always resets the overlay (removed the conditional guard).
- Moved the overlay's touchstart preventDefault out of the
dismissOnOutsideInteraction branch so it fires whenever the overlay
happens to be shown (covering the dialog case in whitelist mode too).
- showOrDefer now only defers for the HTP dialog (.dialog .how-to-play),
not for every dialog. The migration dialog is intentionally excluded so
the banner can appear on top of it rather than waiting until it closes.
- The dismissed check moved from an early return into a new autoShow()
wrapper so that the infrastructure (and the returned API) is always set
up, even when the banner was previously dismissed.
Debug menu Install Banner button (task 2):
- Added "Install Banner" button to #debug-dialog-content template.
- initInstallBanner now returns { showBanner } so callers can force a
re-show independent of the dismissed state.
- Debug handler calls installBannerAPI.showBanner() on click; dismissed
state in localStorage is not touched, so the banner won't re-appear on
reload.
https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
The migration dialog (and any other dialog) will keep the banner deferred until all dialogs are closed before the banner appears. https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
versioned cache names for full PWA installability on Chrome/Android
build time, and wire it into scripts/build.sh
on first visit (permanently dismissed via localStorage)
https://claude.ai/code/session_01CuvPN7bqWC4tkjNkFEtP7j
Summary by CodeRabbit
New Features
Style
Tests
Chore