feat(notifications): dynamic notification sounds with system & custom support#148
feat(notifications): dynamic notification sounds with system & custom support#148YusufLisawi wants to merge 3 commits intocoollabsio:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the prior hardcoded notification sound list with a dynamic, ID-based system that enumerates available system sounds (macOS) and user-imported custom sounds, fetching playable file paths via Tauri backend commands and playing via the asset protocol.
Changes:
- Replaces the static
NotificationSoundunion/options with string-based sound IDs and introducesNotificationSoundEntryfor UI listing/grouping. - Adds a preferences service query hook to fetch notification sounds from the backend and updates the Preferences UI to group sounds and support importing/deleting custom sounds.
- Updates sound playback to resolve backend file paths on-demand (async) and adjusts Tauri CSP/asset protocol scope to allow media playback from asset URLs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/types/preferences.ts |
Updates sound typing to ID-based strings and adds NotificationSoundEntry metadata for UI. |
src/services/preferences.ts |
Adds useNotificationSounds query hook and query keys for dynamic sound listing. |
src/lib/sounds.ts |
Converts playNotificationSound to async backend-resolved playback via asset protocol. |
src/components/preferences/panes/GeneralPane.tsx |
Updates Notifications settings UI to use dynamic sound lists, plus import/delete actions for custom sounds. |
src/components/chat/hooks/useStreamingEvents.ts |
Updates notification sound selection to use string IDs. |
src/App.tsx |
Removes sound preloading on startup. |
src-tauri/tauri.conf.json |
Adds media-src CSP and broadens asset protocol allowlist for macOS system tones. |
src-tauri/src/lib.rs |
Adds Tauri commands to list/get/import/delete notification sounds and manages custom sounds directory. |
| <button | ||
| type="button" | ||
| className="text-muted-foreground hover:text-destructive ml-auto" | ||
| onPointerDown={e => { | ||
| e.stopPropagation() | ||
| e.preventDefault() | ||
| onDelete(s.id) | ||
| }} | ||
| > | ||
| <X className="size-3" /> | ||
| </button> |
There was a problem hiding this comment.
The custom-sound delete control only uses onPointerDown, so keyboard activation (Enter/Space) won’t trigger deletion. Add an onClick/keyboard handler (still preventing Select selection) and provide an accessible name (e.g., aria-label) for the icon-only button.
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={handleImportSound} | ||
| className="gap-1.5" | ||
| > | ||
| <Upload className="size-3.5" /> | ||
| Import Sound | ||
| </Button> |
There was a problem hiding this comment.
“Import Sound” is rendered unconditionally, but handleImportSound depends on Tauri-only APIs (@tauri-apps/plugin-dialog and backend import_custom_sound). In web/non-native mode this will always fail; please hide/disable this button when isNativeApp()/hasBackend() is false.
| export function useNotificationSounds() { | ||
| return useQuery({ | ||
| queryKey: notificationSoundsQueryKeys.list(), | ||
| queryFn: async (): Promise<NotificationSoundEntry[]> => { | ||
| if (!isTauri()) { | ||
| return [{ id: 'none', label: 'None', category: 'default' }] | ||
| } | ||
| return invoke<NotificationSoundEntry[]>('list_notification_sounds') | ||
| }, | ||
| staleTime: 1000 * 60 * 30, // 30 minutes — sound files rarely change | ||
| }) |
There was a problem hiding this comment.
New useNotificationSounds behavior isn’t covered by the existing src/services/preferences.test.ts suite (which already tests usePreferences/useSavePreferences). Please add tests for the non-Tauri fallback and for invoking list_notification_sounds in Tauri context.
| if let Some(filename) = sound_id.strip_prefix("custom:") { | ||
| let custom_dir = get_custom_sounds_dir(&app)?; | ||
| let path = custom_dir.join(filename); | ||
| if path.exists() { | ||
| return Ok(Some(path.to_string_lossy().to_string())); | ||
| } | ||
| return Err(format!("Custom sound not found: {filename}")); |
There was a problem hiding this comment.
get_sound_file_path joins the user-provided custom: filename directly onto the custom sounds directory without validating it. A crafted sound_id like custom:../... could escape the directory. Please apply the same filename/path-traversal validation used in delete_custom_sound (and similarly validate system: names) before building the path.
| const waitingSound = (preferences?.waiting_sound ?? | ||
| 'none') as NotificationSound | ||
| const waitingSound = preferences?.waiting_sound ?? 'none' | ||
| playNotificationSound(waitingSound) |
There was a problem hiding this comment.
Same async-call issue: playNotificationSound returns a Promise but it’s not awaited/handled. Please explicitly handle it (e.g., void playNotificationSound(...)) to avoid lint failures and potential unhandled rejections.
| playNotificationSound(waitingSound) | |
| void playNotificationSound(waitingSound) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Breaking Changes
NotificationSoundtype changed from union type ('none' | 'ding' | 'chime' | 'pop' | 'choochoo') tostring(ID-based:"none"|"system:<name>"|"custom:<filename>")notificationSoundOptionsstatic arrayplayNotificationSound()is now async