Fix stale service worker asset caching#372
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughReplaces inline service-worker scripts with a React ServiceWorkerRegistrar, hardens public/sw.js (fetch/push/notificationclick), updates Next.js cache-control headers for ChangesService Worker Registration Refactor
Sequence DiagramsequenceDiagram
participant Layout
participant Registrar
participant Sync as syncServiceWorker
participant Navigator as navigator.serviceWorker
participant CacheAPI as caches
Layout->>Registrar: render(enabled)
Registrar->>Sync: useEffect -> syncServiceWorker(enabled)
Sync->>Sync: shouldRegisterServiceWorker(enabled, location)
alt Disabled or Invalid Host
Sync->>Navigator: unregister all registrations
Sync->>CacheAPI: delete app caches (emuready_*)
else Enabled and Valid
Sync->>Navigator: unregister unexpected workers (not /sw.js)
Sync->>Navigator: register /sw.js (scope: /, updateViaCache: 'none')
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@public/sw.js`:
- Line 281: Guard against missing notification or data before reading url:
change the code that constructs targetUrl (the line creating targetUrl in the
service worker) to first obtain a safeUrl from event.notification and its data
(e.g., use event.notification && event.notification.data &&
event.notification.data.url or optional chaining event.notification?.data?.url)
falling back to '/' and then call new URL(safeUrl, self.location.origin) to
avoid throwing when notification.data is absent.
- Around line 194-214: The cache update (cache.put(event.request,
modifiedResponse)) is started but not awaited, so the service worker may stop
before it completes; wrap or include the cache.put promise in event.waitUntil so
the worker stays alive until the cache write finishes — e.g., after creating
modifiedResponse and before returning response in the fetchPromise chain, call
event.waitUntil(...) with the cache.put(...) promise (or include the cache.put
in a Promise returned from fetchPromise) while still returning the original
response from fetchPromise; reference fetchPromise, modifiedResponse, cache.put,
event.waitUntil and event.respondWith to locate where to attach the waitUntil.
In `@scripts/sync-version.js`:
- Around line 19-33: The script currently assumes the file contains a matching
CACHE_NAME and a numeric-only version, so replace() can silently no-op; update
versionRegex to accept prerelease characters (e.g. change const versionRegex =
/const CACHE_NAME = 'emuready_v([\d.]+)'/ to a pattern that allows
hyphens/letters like /const CACHE_NAME = 'emuready_v([\dA-Za-z.-]+)'/), and add
a fail-fast check: inside the loop after const match =
content.match(versionRegex) throw a descriptive Error (including target.path and
the expected CACHE_NAME pattern) if !match so the script exits when the service
worker doesn't match the expected pattern; keep the existing replacement/write
logic and updatedTargets push otherwise.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2708fe6f-9f69-4af4-9070-cee41596c081
📒 Files selected for processing (8)
next.config.tspublic/sw-register.jspublic/sw.jsscripts/sync-version.jssrc/app/ServiceWorkerRegistrar.test.tsxsrc/app/ServiceWorkerRegistrar.tsxsrc/app/layout.tsxsrc/app/robots.ts
💤 Files with no reviewable changes (2)
- src/app/robots.ts
- public/sw-register.js
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
1 participant
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.
Description
Fixes #358
This keeps service worker registration in the App Router setup and renames the worker to
/sw.js, matching the Next.js PWA docs. It also removes the old registration script and narrows the cache-control headers so missing Next static assets do not get immutable caching.Type of change
How Has This Been Tested?
Screenshots (if applicable)
N/A
Checklist
Notes for reviewers
After deploy, purge Cloudflare/Vercel cached
/_next/static/*responses, or do a full cache purge, because stale immutable 404s can survive outside the codebase.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests