Skip to content

refactor: enhance notification system with granular controls #48

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.

Sign up for GitHub

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

Open
rtudo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: enhance notification system with granular controls #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions src/services/config-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,26 @@ export class ConfigService {
throw new Error('Invalid JSON in configuration file');
}

// Migrate legacy ntfyUrl to new structure
let migrated = false;
if (fileConfig.interface?.notifications && 'ntfyUrl' in fileConfig.interface.notifications) {
const notifications = fileConfig.interface.notifications as any;
if (!notifications.ntfy) {
notifications.ntfy = {
enabled: false, // Default to false as requested
url: notifications.ntfyUrl
};
}
delete notifications.ntfyUrl;
migrated = true;
this.logger.info('Migrated legacy ntfyUrl to new ntfy configuration structure');
}

// Validate provided fields (strict for provided keys, allow missing)
this.validateProvidedFields(fileConfig);

// Merge with defaults for missing sections while preserving all existing fields (e.g., router)
let updated = false;
let updated = migrated;
const merged: CUIConfig = {
// Start with defaults
...DEFAULT_CONFIG,
Expand Down Expand Up @@ -318,8 +333,17 @@ export class ConfigService {
if (n && typeof n.enabled !== 'boolean') {
throw new Error('Invalid config: interface.notifications.enabled must be a boolean');
}
if (n && n.ntfyUrl !== undefined && typeof n.ntfyUrl !== 'string') {
throw new Error('Invalid config: interface.notifications.ntfyUrl must be a string');
// Validate ntfy settings
if (n && n.ntfy !== undefined) {
if (typeof n.ntfy !== 'object' || n.ntfy === null) {
throw new Error('Invalid config: interface.notifications.ntfy must be an object');
}
if (n.ntfy.enabled !== undefined && typeof n.ntfy.enabled !== 'boolean') {
throw new Error('Invalid config: interface.notifications.ntfy.enabled must be a boolean');
}
if (n.ntfy.url !== undefined && typeof n.ntfy.url !== 'string') {
throw new Error('Invalid config: interface.notifications.ntfy.url must be a string');
}
}
}
}
Expand Down
57 changes: 45 additions & 12 deletions src/services/notification-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,12 @@ export class NotificationService {
*/
private async getNtfyUrl(): Promise<string> {
const config = this.configService.getConfig();
return config.interface.notifications?.ntfyUrl || 'https://ntfy.sh';
return config.interface.notifications?.ntfy?.url || 'https://ntfy.sh';
}

private async isNtfyEnabled(): Promise<boolean> {
const config = this.configService.getConfig();
return config.interface.notifications?.ntfy?.enabled ?? false;
}

/**
Expand Down Expand Up @@ -90,8 +95,18 @@ export class NotificationService {
permissionRequestId: request.id
};

// Send via ntfy
await this.sendNotification(ntfyUrl, topic, notification);
// Send via ntfy if enabled (best-effort, don't fail the whole operation)
if (await this.isNtfyEnabled()) {
try {
await this.sendNotification(ntfyUrl, topic, notification);
} catch (ntfyError) {
this.logger.warn('Ntfy notification failed (non-fatal)', {
error: (ntfyError as Error)?.message,
url: ntfyUrl,
topic
});
}
}

// Also broadcast via native web push (best-effort)
try {
Expand Down Expand Up @@ -152,8 +167,18 @@ export class NotificationService {
streamingId
};

// Send via ntfy
await this.sendNotification(ntfyUrl, topic, notification);
// Send via ntfy if enabled (best-effort, don't fail the whole operation)
if (await this.isNtfyEnabled()) {
try {
await this.sendNotification(ntfyUrl, topic, notification);
} catch (ntfyError) {
this.logger.warn('Ntfy notification failed (non-fatal)', {
error: (ntfyError as Error)?.message,
url: ntfyUrl,
topic
});
}
}

// Also broadcast via native web push (best-effort)
try {
Expand Down Expand Up @@ -210,14 +235,22 @@ export class NotificationService {
headers['X-CUI-PermissionRequestId'] = notification.permissionRequestId;
}

const response = await fetch(url, {
method: 'POST',
headers,
body: notification.message
});
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 5000); // 5 second timeout

try {
const response = await fetch(url, {
method: 'POST',
headers,
body: notification.message,
signal: controller.signal
});

if (!response.ok) {
throw new Error(`Ntfy returned ${response.status}: ${await response.text()}`);
if (!response.ok) {
throw new Error(`Ntfy returned ${response.status}: ${await response.text()}`);
}
} finally {
clearTimeout(timeout);
}
}
}
16 changes: 11 additions & 5 deletions src/services/web-push-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,10 @@ export class WebPushService {
}

getEnabled(): boolean {
const enabled = this.configService.getConfig().interface.notifications?.enabled ?? false;
return enabled;
const notifications = this.configService.getConfig().interface.notifications;
const globalEnabled = notifications?.enabled ?? false;
const webPushEnabled = notifications?.webPush?.enabled ?? true; // Default to true for backward compatibility
return globalEnabled && webPushEnabled;
}

getSubscriptionCount(): number {
Expand Down Expand Up @@ -203,15 +205,19 @@ export class WebPushService {
await webpush.sendNotification(sub, JSON.stringify(payload), { TTL: 60 });
this.upsertSeenStmt.run(new Date().toISOString(), 0, row.endpoint);
sent += 1;
} catch (_err: unknown) {
} catch (err: unknown) {
failed += 1;
// 410 Gone or 404 Not Found => expire subscription
const status = undefined;
const status = (err as any)?.statusCode || (err as any)?.response?.statusCode;
if (status === 404 || status === 410) {
this.upsertSeenStmt.run(new Date().toISOString(), 1, row.endpoint);
this.logger.info('Expired web push subscription removed', { endpoint: row.endpoint, status });
} else {
this.logger.error('Failed sending web push notification', { endpoint: row.endpoint, statusCode: status });
this.logger.error('Failed sending web push notification', {
endpoint: row.endpoint,
statusCode: status,
error: err instanceof Error ? err.message : String(err)
});
}
}
})
Expand Down
6 changes: 5 additions & 1 deletion src/types/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@ export interface InterfaceConfig {
language: string;
notifications?: {
enabled: boolean;
ntfyUrl?: string;
ntfy?: {
enabled: boolean;
url?: string;
};
webPush?: {
enabled?: boolean;
subject?: string; // e.g. mailto:you@example.com
vapidPublicKey?: string;
vapidPrivateKey?: string;
Expand Down
51 changes: 45 additions & 6 deletions src/web/chat/components/PreferencesModal/NotificationTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,24 @@ export function NotificationTab({ prefs, machineId, onUpdate }: Props) {

{/* Ntfy Section */}
<div className="py-4 border-b border-neutral-200 dark:border-neutral-800">
<h3 className="text-sm font-semibold text-neutral-900 dark:text-neutral-100 mb-3">Ntfy</h3>
<div className="flex items-center justify-between mb-3">
<h3 className="text-sm font-semibold text-neutral-900 dark:text-neutral-100">Ntfy</h3>
<Switch
checked={prefs.notifications?.ntfy?.enabled || false}
onCheckedChange={(checked) => onUpdate({
notifications: {
...prefs.notifications,
enabled: prefs.notifications?.enabled || false,
ntfy: {
...prefs.notifications?.ntfy,
enabled: checked
}
}
})}
disabled={!prefs.notifications?.enabled}
aria-label="Toggle ntfy notifications"
/>
</div>
<p className="text-sm text-neutral-600 dark:text-neutral-400 mb-3">
Ntfy works out of the box but opens notifications in the ntfy app. To receive push notifications, subscribe to the following <a href="https://ntfy.sh/" target="_blank" rel="noopener noreferrer" className="text-blue-500 hover:underline">ntfy topic</a>:
</p>
Expand All @@ -171,15 +188,20 @@ export function NotificationTab({ prefs, machineId, onUpdate }: Props) {
id="ntfy-url"
type="url"
className="bg-white dark:bg-neutral-900 border-neutral-200 dark:border-neutral-700 hover:border-neutral-300 dark:hover:border-neutral-600 focus:border-blue-500 dark:focus:border-blue-400"
value={prefs.notifications?.ntfyUrl || ''}
value={prefs.notifications?.ntfy?.url || ''}
placeholder="https://ntfy.sh"
onChange={(e) => onUpdate({
notifications: {
...prefs.notifications,
enabled: prefs.notifications?.enabled || false,
ntfyUrl: e.target.value || undefined
ntfy: {
...prefs.notifications?.ntfy,
enabled: prefs.notifications?.ntfy?.enabled || false,
url: e.target.value || undefined
}
}
})}
disabled={!prefs.notifications?.enabled || !prefs.notifications?.ntfy?.enabled}
aria-label="Ntfy server URL"
/>
</div>
Expand All @@ -188,7 +210,24 @@ export function NotificationTab({ prefs, machineId, onUpdate }: Props) {

{/* Web Push Section */}
<div className="py-4">
<h3 className="text-sm font-semibold text-neutral-900 dark:text-neutral-100 mb-3">Web Push</h3>
<div className="flex items-center justify-between mb-3">
<h3 className="text-sm font-semibold text-neutral-900 dark:text-neutral-100">Web Push</h3>
<Switch
checked={prefs.notifications?.webPush?.enabled || false}
onCheckedChange={(checked) => onUpdate({
notifications: {
...prefs.notifications,
enabled: prefs.notifications?.enabled || false,
webPush: {
...prefs.notifications?.webPush,
enabled: checked
}
}
})}
disabled={!prefs.notifications?.enabled}
aria-label="Toggle web push notifications"
/>
</div>
<p className="text-sm text-neutral-600 dark:text-neutral-400 mb-4">
Web Push requires HTTPS hosting. See setup instructions at{' '}
<a
Expand All @@ -204,7 +243,7 @@ export function NotificationTab({ prefs, machineId, onUpdate }: Props) {
<div className="flex gap-3 mb-4">
<Button
onClick={handleWebPushToggle}
disabled={webPushLoading || !prefs.notifications?.enabled}
disabled={webPushLoading || !prefs.notifications?.enabled || !prefs.notifications?.webPush?.enabled}
variant="outline"
className="flex items-center gap-2"
>
Expand All @@ -228,7 +267,7 @@ export function NotificationTab({ prefs, machineId, onUpdate }: Props) {

<Button
onClick={handleSendTest}
disabled={!webPushSubscription || !prefs.notifications?.enabled}
disabled={!webPushSubscription || !prefs.notifications?.enabled || !prefs.notifications?.webPush?.enabled}
variant="outline"
className="flex items-center gap-2"
>
Expand Down
8 changes: 7 additions & 1 deletion src/web/chat/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,13 @@ export interface Preferences {
language: string;
notifications?: {
enabled: boolean;
ntfyUrl?: string;
ntfy?: {
enabled: boolean;
url?: string;
};
webPush?: {
enabled?: boolean;
};
};
}

Expand Down