Full PWA Support Implementation #436
Conversation
- Added `app/manifest.ts` to generate `manifest.webmanifest`. - Created placeholder PWA icons in `public/`. - Implemented `components/pwa/ios-install-prompt.tsx` for iOS users. - Added a basic Service Worker in `public/sw.js` and registration component `components/pwa/sw-registration.tsx` to satisfy Android installation criteria. - Updated `app/layout.tsx` with PWA metadata and registration components.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
|
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall direction is solid, but there are a few correctness/maintainability pitfalls: the SW registration effect can add duplicate load listeners and logs in production, and the service worker’s no-op fetch handler is risky and should be pass-through or removed. The iOS prompt uses a permanent dismissal flag and includes an any cast that can be easily avoided. iOS icon metadata likely needs an apple-touch-icon for best results.
Additional notes (1)
- Maintainability |
components/pwa/ios-install-prompt.tsx:12-20
This uses(window.navigator as any).standalone, which is an unsafe-but-type-valid escape hatch. Since this is a small, isolated use, it’s better to avoidanyby narrowing via an interface/type guard for maintainability and to prevent accidental misuse elsewhere.
Summary of changes
PWA support added
This PR introduces baseline Progressive Web App support across Next.js App Router:
- Manifest: adds
app/manifest.tsto servemanifest.webmanifestviaMetadataRoute.Manifest, includingdisplay: 'standalone', colors, icons (including amaskableicon), and categories. - Layout metadata: updates
app/layout.tsxto addappleWebAppmetadata andicons, and mounts PWA helper components. - iOS UX: adds
components/pwa/ios-install-prompt.tsx, a client component that detects iOS + non-standalone mode and shows an “Add to Home Screen” instruction UI. - Service worker: adds
public/sw.jspluscomponents/pwa/sw-registration.tsxto register it on page load. - Assets: adds placeholder icons under
public/(icon-192.png,icon-512.png,icon-maskable.png).
Relevant files: app/layout.tsx, app/manifest.ts, components/pwa/*, public/sw.js.
| export function SwRegistration() { | ||
| useEffect(() => { | ||
| if (typeof window !== 'undefined' && 'serviceWorker' in navigator) { | ||
| window.addEventListener('load', () => { | ||
| navigator.serviceWorker | ||
| .register('/sw.js') | ||
| .then((registration) => { | ||
| console.log('SW registered: ', registration) | ||
| }) | ||
| .catch((registrationError) => { | ||
| console.log('SW registration failed: ', registrationError) | ||
| }) | ||
| }) | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
SwRegistration adds a load event listener but never removes it. In React Strict Mode/dev, effects may run more than once, and you can end up with duplicate listeners and duplicate registrations. Also, logging SW registration to the console in production is noisy.
Suggestion
Consider registering immediately (or via a stable handler) and adding cleanup to avoid leaking listeners, and gate logs behind NODE_ENV !== 'production'.
Example:
useEffect(() => {
if (!('serviceWorker' in navigator)) return
const register = () => {
navigator.serviceWorker.register('/sw.js').catch(() => {
// optionally report
})
}
if (document.readyState === 'complete') {
register()
return
}
window.addEventListener('load', register, { once: true })
return () => window.removeEventListener('load', register)
}, [])Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // public/sw.js | ||
| // Basic service worker to satisfy PWA install criteria | ||
| self.addEventListener('install', (event) => { | ||
| self.skipWaiting(); | ||
| }); | ||
|
|
||
| self.addEventListener('activate', (event) => { | ||
| event.waitUntil(clients.claim()); | ||
| }); | ||
|
|
||
| self.addEventListener('fetch', (event) => { | ||
| // A fetch handler is required for the PWA to be installable on Chrome. | ||
| // This is a "no-op" fetch handler. | ||
| }); |
There was a problem hiding this comment.
The service worker has a fetch handler that does nothing. A no-op fetch handler can have unintended effects (it still intercepts all requests and can complicate debugging), and it doesn't provide offline capability. If you only need installability, consider omitting the fetch handler and instead implement minimal caching (or explicitly event.respondWith(fetch(event.request))) so the handler is semantically correct.
Suggestion
Either remove the fetch listener entirely (if install criteria are met without it in your target browsers), or make it explicitly pass-through to avoid surprises.
Example pass-through:
self.addEventListener('fetch', (event) => {
event.respondWith(fetch(event.request))
})If you want minimal offline support, add a cache in install and serve cached assets first for navigation requests.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Check if prompt was dismissed recently | ||
| const isPromptDismissed = localStorage.getItem('iosPwaPromptDismissed') | ||
|
|
||
| if (isIosDevice && !isInStandaloneMode && !isPromptDismissed) { | ||
| setShowPrompt(true) | ||
| } | ||
| }, []) | ||
|
|
||
| const handleDismiss = () => { | ||
| setShowPrompt(false) | ||
| localStorage.setItem('iosPwaPromptDismissed', 'true') | ||
| } |
There was a problem hiding this comment.
The iOS prompt uses a permanent dismissal flag (iosPwaPromptDismissed = true). That means users who dismiss once will never see it again, even months later or after major updates. Typically this should be a TTL (e.g., 7–30 days) or a versioned dismissal key so you can re-prompt after meaningful product changes.
Suggestion
Store a timestamp and only suppress for a period (or version the key):
const KEY = 'iosPwaPromptDismissedAt'
const dismissedAt = Number(localStorage.getItem(KEY) ?? '0')
const THIRTY_DAYS = 30 * 24 * 60 * 60 * 1000
const dismissedRecently = dismissedAt && Date.now() - dismissedAt < THIRTY_DAYS
// ...
localStorage.setItem(KEY, String(Date.now()))Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Detect iOS | ||
| const userAgent = window.navigator.userAgent | ||
| const isIosDevice = /iPad|iPhone|iPod/.test(userAgent) | ||
|
|
||
| // Detect if already in standalone mode (installed) | ||
| const isInStandaloneMode = | ||
| (window.navigator as any).standalone || | ||
| window.matchMedia('(display-mode: standalone)').matches | ||
|
|
||
| // Check if prompt was dismissed recently | ||
| const isPromptDismissed = localStorage.getItem('iosPwaPromptDismissed') | ||
|
|
||
| if (isIosDevice && !isInStandaloneMode && !isPromptDismissed) { | ||
| setShowPrompt(true) | ||
| } | ||
| }, []) | ||
|
|
||
| const handleDismiss = () => { | ||
| setShowPrompt(false) | ||
| localStorage.setItem('iosPwaPromptDismissed', 'true') | ||
| } |
There was a problem hiding this comment.
The iOS install prompt is dismissed permanently (localStorage.setItem('iosPwaPromptDismissed', 'true')). That means users who dismiss once will never see it again, even after weeks/months, which is usually not desired and can reduce install conversions. Also, the iOS detection via /iPad|iPhone|iPod/ misses modern iPadOS Safari in some cases (reports itself closer to macOS); the prompt may not appear on iPads.
Suggestion
Use a TTL-based dismissal and broaden iPadOS detection. Example:
const DISMISS_KEY = 'iosPwaPromptDismissedAt'
const DISMISS_TTL_MS = 1000 * 60 * 60 * 24 * 14 // 14 days
const dismissedAt = Number(localStorage.getItem(DISMISS_KEY) ?? '0')
const isPromptDismissed = Date.now() - dismissedAt < DISMISS_TTL_MS
const ua = navigator.userAgent
const isIphoneIpod = /iPhone|iPod/.test(ua)
const isIpad = /iPad/.test(ua) || (navigator.platform === 'MacIntel' && navigator.maxTouchPoints > 1)
const isIosDevice = isIphoneIpod || isIpad
// on dismiss:
localStorage.setItem(DISMISS_KEY, String(Date.now()))Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| appleWebApp: { | ||
| capable: true, | ||
| statusBarStyle: 'default', | ||
| title: 'QCX' | ||
| }, | ||
| icons: { | ||
| icon: '/icon-192.png', | ||
| apple: '/icon-192.png' | ||
| }, |
There was a problem hiding this comment.
metadata.icons.apple is set to /icon-192.png, but iOS home screen icons are typically expected at specific sizes (e.g., 180x180). Using only a 192px icon may lead to suboptimal rendering on iOS. If you want good iOS support, consider adding an apple-touch-icon at 180x180 (and optionally additional sizes) and reference it via metadata.
Suggestion
Add a dedicated public/apple-touch-icon.png (180x180) and set:
icons: {
icon: [
{ url: '/icon-192.png', sizes: '192x192', type: 'image/png' },
{ url: '/icon-512.png', sizes: '512x512', type: 'image/png' },
],
apple: [{ url: '/apple-touch-icon.png', sizes: '180x180' }],
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||
User description
I have implemented full PWA support following the requirements.
Key changes:
app/manifest.tsusing Next.js 15 conventions. It includes all required fields likedisplay: standalone,theme_color, and amaskableicon.public/(192x192, 512x512, and 1024x1024 maskable).public/sw.js) with afetchhandler, which is required by Chrome to trigger the native installation prompt.IosInstallPromptcomponent that detects iOS devices and guides users to use the "Add to Home Screen" option in the Safari share menu.app/layout.tsxto include the necessary metadata (apple-mobile-web-app-capable) and register the service worker/iOS prompt.The implementation has been verified to correctly serve the manifest and display the iOS prompt when emulating an iPhone.
PR created automatically by Jules for task 17332543261294749333 started by @ngoiyaeric
PR Type
Enhancement
Description
Implements complete PWA support with manifest and icons
Adds iOS install prompt for Safari users
Registers service worker for Android compatibility
Integrates PWA metadata into app layout
Diagram Walkthrough
File Walkthrough
manifest.ts
Web app manifest configurationapp/manifest.ts
icons
sw.js
Basic service worker implementationpublic/sw.js
layout.tsx
PWA metadata and component integrationapp/layout.tsx
style
ios-install-prompt.tsx
iOS installation prompt componentcomponents/pwa/ios-install-prompt.tsx
Screen
sw-registration.tsx
Service worker registration componentcomponents/pwa/sw-registration.tsx