-
-
Notifications
You must be signed in to change notification settings - Fork 191
feat: Implement PWA support #412
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
Co-authored-by: Jean <jean@analoginterface.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 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 |
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.
3 issues found across 17 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/web/public/sw.js">
<violation number="1" location="apps/web/public/sw.js:31">
Normalize `urlToOpen` to an absolute URL before comparing with `client.url`, otherwise the focus branch never runs and every notification click opens a duplicate tab.</violation>
</file>
<file name="apps/web/src/app/manifest.ts">
<violation number="1" location="apps/web/src/app/manifest.ts:15">
The manifest references /icon-192x192.png, but that file is missing from apps/web/public, so the installed PWA will have no icon. Please add the asset or update the path.</violation>
</file>
<file name="packages/db/src/schema/push-subscriptions.ts">
<violation number="1" location="packages/db/src/schema/push-subscriptions.ts:24">
Please enforce a uniqueness constraint on a user/endpoint pair – the subscribe mutation assumes only one row will exist, but concurrent inserts can currently create duplicates.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
apps/web/public/sw.js
Outdated
| event.notification.close(); | ||
|
|
||
| // Open the app at the specified URL or default to home | ||
| const urlToOpen = event.notification.data?.url || "/"; |
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.
Normalize urlToOpen to an absolute URL before comparing with client.url, otherwise the focus branch never runs and every notification click opens a duplicate tab.
Prompt for AI agents
Address the following comment on apps/web/public/sw.js at line 31:
<comment>Normalize `urlToOpen` to an absolute URL before comparing with `client.url`, otherwise the focus branch never runs and every notification click opens a duplicate tab.</comment>
<file context>
@@ -0,0 +1,67 @@
+ event.notification.close();
+
+ // Open the app at the specified URL or default to home
+ const urlToOpen = event.notification.data?.url || "/";
+
+ event.waitUntil(
</file context>
| const urlToOpen = event.notification.data?.url || "/"; | |
| const urlToOpen = new URL(event.notification.data?.url || "/", self.location.origin).href; |
✅ Addressed in a4d4b00
| theme_color: "#000000", | ||
| icons: [ | ||
| { | ||
| src: "/icon-192x192.png", |
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 references /icon-192x192.png, but that file is missing from apps/web/public, so the installed PWA will have no icon. Please add the asset or update the path.
Prompt for AI agents
Address the following comment on apps/web/src/app/manifest.ts at line 15:
<comment>The manifest references /icon-192x192.png, but that file is missing from apps/web/public, so the installed PWA will have no icon. Please add the asset or update the path.</comment>
<file context>
@@ -0,0 +1,40 @@
+ theme_color: "#000000",
+ icons: [
+ {
+ src: "/icon-192x192.png",
+ sizes: "192x192",
+ type: "image/png",
</file context>
| }, | ||
| (table) => [ | ||
| index("push_subscription_user_id_idx").on(table.userId), | ||
| index("push_subscription_endpoint_idx").on(table.endpoint), |
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.
Please enforce a uniqueness constraint on a user/endpoint pair – the subscribe mutation assumes only one row will exist, but concurrent inserts can currently create duplicates.
Prompt for AI agents
Address the following comment on packages/db/src/schema/push-subscriptions.ts at line 24:
<comment>Please enforce a uniqueness constraint on a user/endpoint pair – the subscribe mutation assumes only one row will exist, but concurrent inserts can currently create duplicates.</comment>
<file context>
@@ -0,0 +1,36 @@
+ },
+ (table) => [
+ index("push_subscription_user_id_idx").on(table.userId),
+ index("push_subscription_endpoint_idx").on(table.endpoint),
+ ],
+);
</file context>
✅ Addressed in a4d4b00
|
@tembo implement the review if possible, fix the type errors, run format |
Thanks for implementing the review feedback! I've addressed the three issues:
The changes ensure proper PWA functionality with correct notification routing and data integrity. |
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.
No issues found across 5 files
Description
Implements PWA support for Analog Calendar, replacing server actions with tRPC for push notifications and persisting subscriptions in the database.
Screenshots / Recordings
N/A
Type of Change
Related Areas
Testing
Checklist
Notes
This PR introduces full PWA capabilities, including web app manifest, service worker, and push notifications managed via tRPC and stored in the database. A detailed
PWA_IMPLEMENTATION.mdguide is included.Summary by cubic
Adds full PWA support with app install, a service worker, and push notifications powered by tRPC, with subscriptions stored in the database. Also updates security headers, env validation, and adds simple UI to manage install and notifications.
New Features
apps/web/src/app/manifest.ts)apps/web/public/sw.js)packages/api/src/routers/push-notifications.ts)push_subscriptionwith indexes and a unique user+endpoint constraint (packages/db/src/schema/push-subscriptions.ts)InstallPromptandPushNotificationManagernext.config.ts.env.examplePWA_IMPLEMENTATION.mdandPWA_ICONS_README.mdweb-pushMigration
NEXT_PUBLIC_VAPID_PUBLIC_KEYandVAPID_PRIVATE_KEYbun run db:generatethenbun run db:migrateapps/web/public/Written for commit a4d4b00. Summary will update automatically on new commits.