mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 21:10:43 +00:00
fix: address all review feedback for PWA/Web Push PR
- sw.js: guard cache.put with response.ok to prevent caching error responses - sw.js: skip /api/ and /rpc routes from service worker fetch handler - main.ts: only register service worker in production, unregister in dev - push-web.ts: move setVapidDetails to broadcastWebPush (set once, not per-send) - push-web.ts: auto-remove expired 410 Gone subscriptions per Web Push spec - push-subscription.ts: remove unused detectWebPushState/DEFAULT_STATE - push-subscription.ts: roll back local PushManager on gateway registration failure - push-subscription.ts: always unsubscribe locally even if gateway request fails - app.ts: split initWebPushState and reconcileWebPushState for proper timing - app-gateway.ts: trigger push reconciliation on gateway connect (onHello) - config.ts: fix misleading notification copy (auto-broadcast is not yet implemented) - schema/push.ts: add HTTPS pattern and maxLength to endpoint/key schemas
This commit is contained in:
committed by
Val Alexander
parent
7881924a1b
commit
34c8bfb87e
@@ -31,8 +31,8 @@ export const PushTestResultSchema = Type.Object(
|
||||
|
||||
const WebPushKeysSchema = Type.Object(
|
||||
{
|
||||
p256dh: NonEmptyString,
|
||||
auth: NonEmptyString,
|
||||
p256dh: Type.String({ minLength: 1, maxLength: 512 }),
|
||||
auth: Type.String({ minLength: 1, maxLength: 512 }),
|
||||
},
|
||||
{ additionalProperties: false },
|
||||
);
|
||||
@@ -41,7 +41,7 @@ export const WebPushVapidPublicKeyParamsSchema = Type.Object({}, { additionalPro
|
||||
|
||||
export const WebPushSubscribeParamsSchema = Type.Object(
|
||||
{
|
||||
endpoint: Type.String({ minLength: 1, maxLength: 2048 }),
|
||||
endpoint: Type.String({ minLength: 1, maxLength: 2048, pattern: "^https://" }),
|
||||
keys: WebPushKeysSchema,
|
||||
},
|
||||
{ additionalProperties: false },
|
||||
@@ -49,7 +49,7 @@ export const WebPushSubscribeParamsSchema = Type.Object(
|
||||
|
||||
export const WebPushUnsubscribeParamsSchema = Type.Object(
|
||||
{
|
||||
endpoint: Type.String({ minLength: 1, maxLength: 2048 }),
|
||||
endpoint: Type.String({ minLength: 1, maxLength: 2048, pattern: "^https://" }),
|
||||
},
|
||||
{ additionalProperties: false },
|
||||
);
|
||||
|
||||
@@ -151,10 +151,10 @@ export async function registerWebPushSubscription(
|
||||
const { endpoint, keys, baseDir } = params;
|
||||
|
||||
if (!isValidEndpoint(endpoint)) {
|
||||
throw new Error("invalid push subscription endpoint");
|
||||
throw new Error("Invalid push subscription endpoint: must be an HTTPS URL under 2048 chars");
|
||||
}
|
||||
if (!isValidKey(keys.p256dh) || !isValidKey(keys.auth)) {
|
||||
throw new Error("invalid push subscription keys");
|
||||
throw new Error("Invalid push subscription keys: must be non-empty strings under 512 chars");
|
||||
}
|
||||
|
||||
return await withLock(async () => {
|
||||
@@ -244,8 +244,6 @@ export async function sendWebPushNotification(
|
||||
): Promise<WebPushSendResult> {
|
||||
const keys = vapidKeys ?? (await resolveVapidKeys());
|
||||
|
||||
webPush.setVapidDetails(keys.subject, keys.publicKey, keys.privateKey);
|
||||
|
||||
const pushSubscription = {
|
||||
endpoint: subscription.endpoint,
|
||||
keys: {
|
||||
@@ -289,11 +287,15 @@ export async function broadcastWebPush(
|
||||
}
|
||||
|
||||
const vapidKeys = await resolveVapidKeys(baseDir);
|
||||
|
||||
// Set VAPID details once before fanning out concurrent sends.
|
||||
webPush.setVapidDetails(vapidKeys.subject, vapidKeys.publicKey, vapidKeys.privateKey);
|
||||
|
||||
const results = await Promise.allSettled(
|
||||
subscriptions.map((sub) => sendWebPushNotification(sub, payload, vapidKeys)),
|
||||
);
|
||||
|
||||
return results.map((r, i) =>
|
||||
const mapped = results.map((r, i) =>
|
||||
r.status === "fulfilled"
|
||||
? r.value
|
||||
: {
|
||||
@@ -302,4 +304,18 @@ export async function broadcastWebPush(
|
||||
error: r.reason instanceof Error ? r.reason.message : "unknown error",
|
||||
},
|
||||
);
|
||||
|
||||
// Clean up expired subscriptions (HTTP 410 Gone) per Web Push spec.
|
||||
const expiredEndpoints = mapped
|
||||
.map((result, i) => ({ result, sub: subscriptions[i] }))
|
||||
.filter(({ result }) => !result.ok && result.statusCode === 410)
|
||||
.map(({ sub }) => sub.endpoint);
|
||||
|
||||
if (expiredEndpoints.length > 0) {
|
||||
await Promise.allSettled(
|
||||
expiredEndpoints.map((endpoint) => clearWebPushSubscriptionByEndpoint(endpoint, baseDir)),
|
||||
);
|
||||
}
|
||||
|
||||
return mapped;
|
||||
}
|
||||
|
||||
@@ -30,15 +30,22 @@ self.addEventListener("fetch", (event) => {
|
||||
return;
|
||||
}
|
||||
|
||||
// Network-first for HTML / API; cache-first for hashed assets.
|
||||
// Skip API requests — they should never be cached.
|
||||
if (url.pathname.startsWith("/api/") || url.pathname.startsWith("/rpc")) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Cache-first for hashed assets; network-first for HTML/other.
|
||||
if (url.pathname.includes("/assets/")) {
|
||||
event.respondWith(
|
||||
caches.match(event.request).then(
|
||||
(cached) =>
|
||||
cached ||
|
||||
fetch(event.request).then((response) => {
|
||||
const clone = response.clone();
|
||||
void caches.open(CACHE_NAME).then((cache) => cache.put(event.request, clone));
|
||||
if (response.ok) {
|
||||
const clone = response.clone();
|
||||
void caches.open(CACHE_NAME).then((cache) => cache.put(event.request, clone));
|
||||
}
|
||||
return response;
|
||||
}),
|
||||
),
|
||||
@@ -47,8 +54,10 @@ self.addEventListener("fetch", (event) => {
|
||||
event.respondWith(
|
||||
fetch(event.request)
|
||||
.then((response) => {
|
||||
const clone = response.clone();
|
||||
void caches.open(CACHE_NAME).then((cache) => cache.put(event.request, clone));
|
||||
if (response.ok) {
|
||||
const clone = response.clone();
|
||||
void caches.open(CACHE_NAME).then((cache) => cache.put(event.request, clone));
|
||||
}
|
||||
return response;
|
||||
})
|
||||
.catch(() => caches.match(event.request)),
|
||||
|
||||
@@ -1,6 +1,13 @@
|
||||
import "./styles.css";
|
||||
import "./ui/app.ts";
|
||||
|
||||
if ("serviceWorker" in navigator) {
|
||||
if (import.meta.env?.PROD && "serviceWorker" in navigator) {
|
||||
void navigator.serviceWorker.register("./sw.js");
|
||||
} else if (!import.meta.env?.PROD && "serviceWorker" in navigator) {
|
||||
// Unregister any leftover dev SW to avoid stale cache issues.
|
||||
void navigator.serviceWorker.getRegistrations().then((registrations) => {
|
||||
for (const r of registrations) {
|
||||
void r.unregister();
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@@ -339,6 +339,8 @@ export function connectGateway(host: GatewayHost, options?: ConnectGatewayOption
|
||||
void loadNodes(host as unknown as NodesState, { quiet: true });
|
||||
void loadDevices(host as unknown as DevicesState, { quiet: true });
|
||||
void refreshActiveTab(host as unknown as Parameters<typeof refreshActiveTab>[0]);
|
||||
// Re-run push reconciliation now that the gateway client is available.
|
||||
void (host as unknown as OpenClawApp).reconcileWebPushState();
|
||||
},
|
||||
onClose: ({ code, reason, error }) => {
|
||||
if (host.client !== client) {
|
||||
|
||||
@@ -963,31 +963,36 @@ export class OpenClawApp extends LitElement {
|
||||
try {
|
||||
const { getExistingSubscription } = await import("./push-subscription.ts");
|
||||
const existing = await getExistingSubscription();
|
||||
if (existing && this.client) {
|
||||
// Re-register with the gateway to reconcile local/server state.
|
||||
// Handles the case where the gateway lost the subscription (e.g.
|
||||
// state-dir reset) but the browser still has one locally.
|
||||
const subJson = existing.toJSON();
|
||||
if (subJson.endpoint && subJson.keys?.p256dh && subJson.keys?.auth) {
|
||||
try {
|
||||
await this.client.request("push.web.subscribe", {
|
||||
endpoint: subJson.endpoint,
|
||||
keys: { p256dh: subJson.keys.p256dh, auth: subJson.keys.auth },
|
||||
});
|
||||
} catch {
|
||||
// Best-effort — don't block init if gateway is unreachable.
|
||||
}
|
||||
}
|
||||
this.webPushSubscribed = true;
|
||||
} else {
|
||||
this.webPushSubscribed = existing !== null;
|
||||
}
|
||||
this.webPushSubscribed = existing !== null;
|
||||
} catch {
|
||||
// ignore — just means we can't check
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/** Re-register local push subscription with the gateway after connect. */
|
||||
async reconcileWebPushState() {
|
||||
if (!this.webPushSubscribed || !this.client) {
|
||||
return;
|
||||
}
|
||||
try {
|
||||
const { getExistingSubscription } = await import("./push-subscription.ts");
|
||||
const existing = await getExistingSubscription();
|
||||
if (!existing) {
|
||||
return;
|
||||
}
|
||||
const subJson = existing.toJSON();
|
||||
if (subJson.endpoint && subJson.keys?.p256dh && subJson.keys?.auth) {
|
||||
await this.client.request("push.web.subscribe", {
|
||||
endpoint: subJson.endpoint,
|
||||
keys: { p256dh: subJson.keys.p256dh, auth: subJson.keys.auth },
|
||||
});
|
||||
}
|
||||
} catch {
|
||||
// Best-effort — don't block if gateway is unreachable.
|
||||
}
|
||||
}
|
||||
|
||||
async handleWebPushSubscribe() {
|
||||
if (!this.client || this.webPushLoading) {
|
||||
return;
|
||||
|
||||
@@ -7,29 +7,6 @@ export type WebPushState = {
|
||||
loading: boolean;
|
||||
};
|
||||
|
||||
const DEFAULT_STATE: WebPushState = {
|
||||
supported: false,
|
||||
permission: "unsupported",
|
||||
subscribed: false,
|
||||
loading: false,
|
||||
};
|
||||
|
||||
export function detectWebPushState(): WebPushState {
|
||||
const supported =
|
||||
"serviceWorker" in navigator && "PushManager" in window && "Notification" in window;
|
||||
|
||||
if (!supported) {
|
||||
return DEFAULT_STATE;
|
||||
}
|
||||
|
||||
return {
|
||||
supported: true,
|
||||
permission: Notification.permission,
|
||||
subscribed: false,
|
||||
loading: false,
|
||||
};
|
||||
}
|
||||
|
||||
/** Timeout (ms) for service-worker readiness. */
|
||||
const SW_READY_TIMEOUT = 10_000;
|
||||
|
||||
@@ -75,7 +52,8 @@ export async function getExistingSubscription(): Promise<PushSubscription | null
|
||||
* Subscribe to web push notifications.
|
||||
* Requests notification permission if not already granted, fetches VAPID key
|
||||
* from the gateway, subscribes with the PushManager, and registers with the
|
||||
* gateway.
|
||||
* gateway. If gateway registration fails, the local PushManager subscription
|
||||
* is rolled back to avoid local/server state divergence.
|
||||
*/
|
||||
export async function subscribeToWebPush(
|
||||
client: GatewayBrowserClient,
|
||||
@@ -105,31 +83,46 @@ export async function subscribeToWebPush(
|
||||
throw new Error("Invalid push subscription from browser");
|
||||
}
|
||||
|
||||
// Register with gateway.
|
||||
const registerRes = await client.request("push.web.subscribe", {
|
||||
endpoint: subJson.endpoint,
|
||||
keys: {
|
||||
p256dh: subJson.keys.p256dh,
|
||||
auth: subJson.keys.auth,
|
||||
},
|
||||
});
|
||||
// Register with gateway — roll back local subscription on failure.
|
||||
try {
|
||||
const registerRes = await client.request("push.web.subscribe", {
|
||||
endpoint: subJson.endpoint,
|
||||
keys: {
|
||||
p256dh: subJson.keys.p256dh,
|
||||
auth: subJson.keys.auth,
|
||||
},
|
||||
});
|
||||
|
||||
return registerRes as { subscriptionId: string };
|
||||
return registerRes as { subscriptionId: string };
|
||||
} catch (err) {
|
||||
// Gateway registration failed — unsubscribe locally to keep state consistent.
|
||||
try {
|
||||
await pushSubscription.unsubscribe();
|
||||
} catch {
|
||||
// Best-effort rollback.
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Unsubscribe from web push notifications.
|
||||
* Always unsubscribes locally even if the gateway request fails, to avoid
|
||||
* leaving the browser subscribed with no server-side record.
|
||||
*/
|
||||
export async function unsubscribeFromWebPush(client: GatewayBrowserClient): Promise<void> {
|
||||
const registration = await swReady();
|
||||
const subscription = await registration.pushManager.getSubscription();
|
||||
|
||||
if (subscription) {
|
||||
// Notify gateway first.
|
||||
await client.request("push.web.unsubscribe", {
|
||||
endpoint: subscription.endpoint,
|
||||
});
|
||||
// Then unsubscribe locally.
|
||||
// Notify gateway (best-effort — always unsubscribe locally afterward).
|
||||
try {
|
||||
await client.request("push.web.unsubscribe", {
|
||||
endpoint: subscription.endpoint,
|
||||
});
|
||||
} catch {
|
||||
// Gateway may be unreachable; still unsubscribe locally.
|
||||
}
|
||||
await subscription.unsubscribe();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user