-
-
Notifications
You must be signed in to change notification settings - Fork 7
Full PWA Support Implementation #435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Created `app/manifest.ts` with PWA metadata (name, icons, display, etc.). - Created placeholder icons in `public/` (192, 512, 1024-maskable). - Added `components/pwa/ios-install-prompt.tsx` for iOS users. - Updated `app/layout.tsx` with metadata for PWA and integrated the iOS prompt. - Verified manifest generation and iOS prompt display (via Playwright with mocked UA). - Added `apple-icon.png` in `app/` for automatic Apple touch icon generation. - Added `themeColor` to viewport metadata in layout.
|
👋 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 |
|
|
- Corrected 'purpose' field in `app/manifest.ts` to use single values supported by Next.js TypeScript definitions. - Verified build success with `bun run build`. - Added `apple-icon.png` for automatic Apple touch icon generation. - Integrated `IOSInstallPrompt` for improved mobile UX.
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 |
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PWA implementation is generally solid, but the iOS prompt logic is somewhat brittle due to UA parsing and any casts, and the dismiss behavior may be more intrusive than intended due to sessionStorage. The prompt’s close button should be made accessible with an explicit label. The manifest would be more broadly compatible if it included a maskable 512x512 icon, not just a 1024 one.
Additional notes (1)
- Maintainability |
components/pwa/ios-install-prompt.tsx:18-29
The dismiss state is stored insessionStorage, so the prompt will reappear on every new tab/session. If the goal is “non-intrusive,” persisting the dismissal inlocalStorage(optionally with a TTL) tends to better match user expectations and reduce repeated prompts.
Summary of changes
PWA enablement (manifest + iOS UX)
- Added a dynamic Web App Manifest via
app/manifest.ts(MetadataRoute.Manifest) includingname,short_name, icons, colors, screenshots, and categories. - Updated
app/layout.tsxmetadata with PWA/iOS-related settings:appleWebApp(capable,statusBarStyle,title)formatDetection.telephone = false- set
viewport.themeColor = '#171717' - updated
title/description
- Introduced
components/pwa/ios-install-prompt.tsx(client component) that detects iOS + non-standalone mode and shows a dismissible “Add to Home Screen” banner. - Added icon assets:
public/icon-192.png,public/icon-512.png,public/icon-1024-maskable.png, plusapp/apple-icon.pngfor Apple touch icon generation.
| useEffect(() => { | ||
| // Check if it's iOS | ||
| const isIOS = /iPad|iPhone|iPod/.test(navigator.userAgent) && !(window as any).MSStream | ||
|
|
||
| // Check if it's already in standalone mode (installed) | ||
| const isStandalone = window.matchMedia('(display-mode: standalone)').matches || | ||
| (window.navigator as any).standalone | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iOS detection and standalone checks rely on navigator.userAgent and (window as any) casts. This works, but it’s brittle (UA spoofing, iPadOS desktop UA changes) and the any casts reduce safety. You can make this more robust by using feature-based checks and avoiding any (e.g., use navigator.standalone via typed narrowing, and use navigator.userAgentData when available).
Suggestion
Consider replacing UA parsing and any casts with a more robust, typed approach:
useEffect(() => {
const ua = navigator.userAgent
const isIOS =
/iPad|iPhone|iPod/.test(ua) ||
// iPadOS sometimes reports as Mac; detect touch support
(ua.includes('Mac') && 'ontouchend' in document)
const isStandalone =
window.matchMedia('(display-mode: standalone)').matches ||
// iOS Safari (installed) exposes `navigator.standalone`
('standalone' in navigator && (navigator as Navigator & { standalone?: boolean }).standalone)
const dismissed = sessionStorage.getItem('ios-pwa-prompt-dismissed') === 'true'
if (isIOS && !isStandalone && !dismissed) setShowPrompt(true)
}, [])This keeps behavior but reduces brittleness and removes (window as any).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| <button | ||
| onClick={handleDismiss} | ||
| className="absolute top-2 right-2 text-muted-foreground hover:text-foreground" | ||
| > | ||
| <X className="h-4 w-4" /> | ||
| </button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The close control is implemented as a plain <button> with only an icon. For accessibility, it should have an explicit accessible name (e.g., aria-label). Without it, screen readers may announce it ambiguously.
Suggestion
Add an accessible label (and optionally set type="button" explicitly):
<button
type="button"
aria-label="Dismiss install prompt"
onClick={handleDismiss}
className="absolute top-2 right-2 text-muted-foreground hover:text-foreground"
>
<X className="h-4 w-4" />
</button>Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| icons: [ | ||
| { | ||
| src: '/icon-192.png', | ||
| sizes: '192x192', | ||
| type: 'image/png', | ||
| purpose: 'any', | ||
| }, | ||
| { | ||
| src: '/icon-512.png', | ||
| sizes: '512x512', | ||
| type: 'image/png', | ||
| purpose: 'any', | ||
| }, | ||
| { | ||
| src: '/icon-1024-maskable.png', | ||
| sizes: '1024x1024', | ||
| type: 'image/png', | ||
| purpose: 'maskable', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manifest defines a maskable icon only at 1024x1024 and the other icons are purpose: 'any'. Many platforms (notably Android) strongly prefer a maskable 512x512 icon for adaptive launcher shapes, and having maskable only at 1024 may not be used as expected by all install surfaces.
Suggestion
Add a 512x512 maskable variant (and keep any variants). For example, add public/icon-512-maskable.png and include it:
icons: [
{ src: '/icon-192.png', sizes: '192x192', type: 'image/png', purpose: 'any' },
{ src: '/icon-512.png', sizes: '512x512', type: 'image/png', purpose: 'any' },
{ src: '/icon-512-maskable.png', sizes: '512x512', type: 'image/png', purpose: 'maskable' },
{ src: '/icon-1024-maskable.png', sizes: '1024x1024', type: 'image/png', purpose: 'maskable' },
],Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
|
|
||
| const title = '' | ||
| const title = 'QCX - AI-powered Search' | ||
| const description = | ||
| 'language to Maps' | ||
| 'A minimalistic AI-powered search tool that uses advanced models for deep analysis and geospatial data.' | ||
|
|
||
| export const metadata: Metadata = { | ||
| metadataBase: new URL('https://www.qcx.world'), | ||
| title, | ||
| description, | ||
| appleWebApp: { | ||
| capable: true, | ||
| statusBarStyle: 'default', | ||
| title: 'QCX', | ||
| }, | ||
| formatDetection: { | ||
| telephone: false, | ||
| }, | ||
| openGraph: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title is currently a plain string ('QCX - AI-powered Search'), while the manifest uses a different name ('QCX - AI-powered Search & Analysis') and appleWebApp.title uses 'QCX'. This mismatch can create inconsistent install/UI labels across platforms and metadata previews.
Consider centralizing these values (e.g., a shared APP_NAME/APP_DESCRIPTION constant) so metadata, manifest(), and appleWebApp.title stay aligned.
Suggestion
Unify the app naming/description by extracting constants shared by both app/layout.tsx and app/manifest.ts (e.g., lib/app-metadata.ts). Example:
// lib/app-metadata.ts
export const APP_TITLE = 'QCX'
export const APP_TAGLINE = 'AI-powered Search & Analysis'
export const APP_DESCRIPTION = 'A minimalistic AI-powered search tool that uses advanced models for deep analysis and geospatial data.'Then in layout/manifest:
const title = `${APP_TITLE} - ${APP_TAGLINE}`Reply with "@CharlieHelps yes please" if you’d like me to add a commit that consolidates these strings and updates both files accordingly.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
User description
I have implemented full Progressive Web App (PWA) support for the QCX application, following the standards of modern apps like scira.ai.
Key Changes:
app/manifest.tswhich dynamically generates the Web App Manifest. It includes:standalonefor an app-like experience without browser UI.maskablepurpose for Android adaptive icons.public/directory by adapting the existing logo.app/apple-icon.pngto let Next.js automatically generate the necessary Apple touch icons.IOSInstallPromptcomponent that detects iOS devices and shows a non-intrusive banner guiding users on how to "Add to Home Screen" via the Share menu (since iOS doesn't have an automatic prompt).app/layout.tsxwithappleWebAppmetadata (capable: true).themeColor: '#171717'to the viewport configuration to match the app's splash screen and status bar.Verification:
bun run build).manifest.webmanifestis generated.PR created automatically by Jules for task 3991291010368248876 started by @ngoiyaeric
PR Type
Enhancement
Description
Implemented full PWA support with Web App Manifest configuration
Added iOS-specific install prompt for non-Safari app installation
Enhanced metadata with PWA capabilities and theme color settings
Created placeholder icons for multiple device sizes and adaptive icons
Diagram Walkthrough
File Walkthrough
manifest.ts
PWA manifest with icons and metadataapp/manifest.ts
layout.tsx
Enhanced layout with PWA metadata and iOS promptapp/layout.tsx
appleWebAppconfiguration for iOS PWA supportIOSInstallPromptcomponent into layoutthemeColorto viewport for splash screen consistencyformatDetectionto disable telephone number detectionios-install-prompt.tsx
iOS install prompt component with device detectioncomponents/pwa/ios-install-prompt.tsx