Add custom PWA web push notifications#1132
Add custom PWA web push notifications#1132claude-do wants to merge 16 commits intopingdotgg:mainfrom
Conversation
Co-authored-by: Liam C. (do-box) <liam@chaintail.xyz>
Add dev runtime branding assets
…solidation-20260316
Co-authored-by: Liam C. (do-box) <liam@chaintail.xyz>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
github-actions
bot
added
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| Effect.tryPromise<SendResult, WebPushDeliveryError>({ | ||
| try: () => webPush.sendNotification(input.subscription, input.payload), | ||
| catch: (error) => | ||
| new WebPushDeliveryError({ | ||
| message: error instanceof Error ? error.message : String(error), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🟠 High Layers/WebPushNotifications.ts:129
isPermanentDeliveryError receives a WebPushDeliveryError instance, which only has a message property. The original web-push error with statusCode is discarded at lines 131-134, so the check at line 178 always returns false. Subscriptions returning 404/410 are never deleted and are incorrectly treated as transient failures.
const notifySubscription = (input: {
readonly subscription: PushSubscription;
readonly payload: string;
}) =>
Effect.tryPromise<SendResult, WebPushDeliveryError>({
try: () => webPush.sendNotification(input.subscription, input.payload),
catch: (error) =>
new WebPushDeliveryError({
- message: error instanceof Error ? error.message : String(error),
+ message: error instanceof Error ? error.message : String(error),
+ statusCode:
+ typeof error === "object" &&
+ error !== null &&
+ "statusCode" in error
+ ? (error as { statusCode: number }).statusCode
+ : undefined,
}),
});Also found in 1 other location(s)
apps/server/src/wsServer.ts:742
The
isWebPushRequestError(error)check at line 742 will never be true for two reasons: (1) the error is wrapped in aFiberFailurebyEffect.runPromise, and (2)WebPushRequestErrorinstances are already mapped toRouteRequestErrorat lines 520-525 before propagating. This means web push errors that should return 409 or 400 will instead return 500.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/notifications/Layers/WebPushNotifications.ts around lines 129-135:
`isPermanentDeliveryError` receives a `WebPushDeliveryError` instance, which only has a `message` property. The original web-push error with `statusCode` is discarded at lines 131-134, so the check at line 178 always returns `false`. Subscriptions returning 404/410 are never deleted and are incorrectly treated as transient failures.
Evidence trail:
apps/server/src/notifications/Layers/WebPushNotifications.ts lines 20-24 (WebPushDeliveryError class with only `message` property), lines 33-41 (`isPermanentDeliveryError` checking for `statusCode` property), lines 130-135 (error wrapping that discards `statusCode`), line ~177 (call to `isPermanentDeliveryError` with `WebPushDeliveryError` instance)
Also found in 1 other location(s):
- apps/server/src/wsServer.ts:742 -- The `isWebPushRequestError(error)` check at line 742 will never be true for two reasons: (1) the error is wrapped in a `FiberFailure` by `Effect.runPromise`, and (2) `WebPushRequestError` instances are already mapped to `RouteRequestError` at lines 520-525 before propagating. This means web push errors that should return 409 or 400 will instead return 500.
| try { | ||
| const response = await fetch(request); | ||
| const cache = await caches.open(APP_SHELL_CACHE); | ||
| await cache.put(APP_SHELL_URL, response.clone()); | ||
| return response; | ||
| } catch { | ||
| const cachedShell = await caches.match(APP_SHELL_URL); | ||
| return cachedShell ?? Response.error(); | ||
| } |
There was a problem hiding this comment.
🟠 High public/service-worker.js:83
The fetch handler caches HTTP error responses (4xx, 5xx) as the app shell. When fetch(request) returns a non-2xx response (e.g., navigating to a 404 page), that error response is cached under APP_SHELL_URL, corrupting the cache. On subsequent offline navigations, users are served the cached error page instead of the real shell. Add a check like response.ok before caching the response.
try {
const response = await fetch(request);
- const cache = await caches.open(APP_SHELL_CACHE);
- await cache.put(APP_SHELL_URL, response.clone());
- return response;
+ if (response.ok) {
+ const cache = await caches.open(APP_SHELL_CACHE);
+ await cache.put(APP_SHELL_URL, response.clone());
+ }
+ return response;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/public/service-worker.js around lines 83-91:
The fetch handler caches HTTP error responses (4xx, 5xx) as the app shell. When `fetch(request)` returns a non-2xx response (e.g., navigating to a 404 page), that error response is cached under `APP_SHELL_URL`, corrupting the cache. On subsequent offline navigations, users are served the cached error page instead of the real shell. Add a check like `response.ok` before caching the response.
Evidence trail:
apps/web/public/service-worker.js lines 83-91 at REVIEWED_COMMIT: The fetch handler performs `const response = await fetch(request)` followed by `await cache.put(APP_SHELL_URL, response.clone())` without any `response.ok` check between them. HTTP error responses (4xx, 5xx) do not throw exceptions - they resolve normally and would be cached.
| return createNativeSpeechController().isSupported(); | ||
| } | ||
|
|
||
| export function getSnapshot(): TtsSnapshot { |
There was a problem hiding this comment.
🟡 Medium tts/tts.ts:66
getSnapshot returns a new object literal { ...snapshot, status: "unsupported" } on every call when TTS is unsupported. This violates the useSyncExternalStore contract that requires getSnapshot to return referentially equal values when the store data hasn't changed, causing unnecessary re-renders and potential tearing in concurrent React renders. Consider caching the "unsupported" snapshot or returning the same frozen object each time.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/features/tts/tts.ts around line 66:
`getSnapshot` returns a new object literal `{ ...snapshot, status: "unsupported" }` on every call when TTS is unsupported. This violates the `useSyncExternalStore` contract that requires `getSnapshot` to return referentially equal values when the store data hasn't changed, causing unnecessary re-renders and potential tearing in concurrent React renders. Consider caching the "unsupported" snapshot or returning the same frozen object each time.
Evidence trail:
apps/web/src/features/tts/tts.ts lines 67-73 (REVIEWED_COMMIT): `getSnapshot` returns `{ ...snapshot, status: "unsupported" }` - a new object literal - when `!isSupported() && snapshot.status === "idle"`.
apps/web/src/features/tts/useMessageTts.ts line 6 (REVIEWED_COMMIT): Confirms `getSnapshot` is used with `useSyncExternalStore`.
React documentation for `useSyncExternalStore`: https://react.dev/reference/react/useSyncExternalStore states getSnapshot must return a cached value when store hasn't changed.
|
@mcorrig4 Hey! This PR is very large, and contains multiple different fixes/changes in one. Could you please split this into small, targeted PR's? As it is now it is far too large and unfocused for someone to review. I would also recommend creating issues for any bugs or features you may want to implement so there can be community discussion. |
3 participants
Summary
Verification
Note
Add PWA web push notifications with service worker, server VAPID delivery, and subscription management
usePushNotifications, and a Settings UI for enable/disable.web-pushlibrary in WebPushNotifications.ts, aweb_push_subscriptionsDB table (migration 014), and HTTP endpointsGET/PUT/DELETE /api/web-push/...in wsServer.ts.t3-devhost variant; the Vite dev server proxies/api/web-pushto the backend origin fromVITE_WS_URL.ProviderService.respondToUserInputnow fails fast with a validation error when the provider session is inactive, and the client clears the pending input entry when the failure detail indicates expiry.📊 Macroscope summarized 6e339ce. 57 files reviewed, 11 issues evaluated, 2 issues filtered, 3 comments posted
🗂️ Filtered Issues
apps/server/src/wsServer.ts — 0 comments posted, 2 evaluated, 1 filtered
isWebPushRequestError(error)check at line 742 will never be true for two reasons: (1) the error is wrapped in aFiberFailurebyEffect.runPromise, and (2)WebPushRequestErrorinstances are already mapped toRouteRequestErrorat lines 520-525 before propagating. This means web push errors that should return 409 or 400 will instead return 500. [ Cross-file consolidated ]apps/web/public/service-worker.js — 1 comment posted, 3 evaluated, 1 filtered
APP_SHELL_ASSETSincludes both production and dev variants of assets (e.g.,/manifest.webmanifestand/manifest-t3-dev.webmanifest,/favicon.icoand/favicon-dev.ico).cache.addAll()at line 52 requires all fetches to return successful (2xx) responses—if any asset returns 404, the entire service worker installation fails. In a production environment where dev assets don't exist (or vice versa), the service worker will fail to install. [ Failed validation ]