feat(desktop): add custom notification sound#296
Conversation
Replace the default system notification sound with a custom shamisen sound for agent completion and permission request notifications. - Add notification-sound.ts utility to play custom sounds cross-platform - Set notifications to silent and play custom sound instead - Include notification.mp3 with fade-out effect in resources 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a new desktop notification sound module and calls it from the main window notification code; native notification sound is disabled and playback is handled via platform-specific commands with development/production path resolution and a missing-file guard. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/main/windows/main.ts (1)
3-3: Remove unused import.The
appimport from electron is not used anywhere in this file. It's used innotification-sound.ts, but not needed here.Apply this diff to remove the unused import:
-import { Notification, app, screen } from "electron"; +import { Notification, screen } from "electron";apps/desktop/src/main/lib/notification-sound.ts (1)
32-41: Add error handling for better reliability.The
execcalls don't include error callbacks, which means playback failures will be silent. Adding error handling would improve debugging and reliability.Apply this diff to add error callbacks:
if (process.platform === "darwin") { - exec(`afplay "${soundPath}"`); + exec(`afplay "${soundPath}"`, (error) => { + if (error) { + console.warn("[notification-sound] Failed to play sound:", error.message); + } + }); } else if (process.platform === "win32") { exec( `powershell -c "(New-Object Media.SoundPlayer '${soundPath}').PlaySync()"`, + (error) => { + if (error) { + console.warn("[notification-sound] Failed to play sound:", error.message); + } + }, ); } else { // Linux - try common audio players - exec(`paplay "${soundPath}" || aplay "${soundPath}"`); + exec(`paplay "${soundPath}" || aplay "${soundPath}"`, (error) => { + if (error) { + console.warn("[notification-sound] Failed to play sound:", error.message); + } + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/desktop/src/resources/sounds/notification.mp3is excluded by!**/*.mp3
📒 Files selected for processing (2)
apps/desktop/src/main/lib/notification-sound.ts(1 hunks)apps/desktop/src/main/windows/main.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/main/windows/main.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/main/windows/main.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/main/windows/main.ts
🧠 Learnings (4)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/lib/**/*.{ts,tsx} : Never import Node.js modules like node:fs, node:path, node:os in src/lib/electron-router-dom.ts or similar shared code
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules in renderer process or shared code - only in src/main/
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/main/windows/main.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/windows/main.ts (1)
apps/desktop/src/main/lib/notification-sound.ts (1)
playNotificationSound(24-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/main/windows/main.ts (1)
90-93: LGTM!The integration of the custom notification sound is well-structured. Setting
silent: trueprevents the native notification sound from playing, and callingplayNotificationSound()before showing the notification provides a good user experience.apps/desktop/src/main/lib/notification-sound.ts (1)
10-18: LGTM!The path resolution logic correctly handles both development and production environments using Electron's standard APIs.
Replace exec with execFile for playing notification sounds. execFile doesn't spawn a shell, making it immune to command injection attacks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/main/windows/main.ts (1)
79-107: Avoid fully silent notifications when custom sound cannot playRight now, notifications are created with
silent: trueand rely entirely onplayNotificationSound()for audio, butplayNotificationSoundcan no-op (missing file, missing player, spawn error). In that case the user gets a visually-delivered notification with no sound, whereas previously the OS default sound would have played.Consider one of:
- Having
playNotificationSound()return a boolean and only settingsilent: truewhen it successfully initiates playback, otherwise leavingsilent: falseto fall back to the OS sound.- If you want sounds even where
Notification.isSupported()is false, decouple the sound call from that guard so audio can still play in environments without desktop notifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/notification-sound.ts(1 hunks)apps/desktop/src/main/windows/main.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/notification-sound.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/windows/main.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/windows/main.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/main/windows/main.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/windows/main.ts (1)
apps/desktop/src/main/lib/notification-sound.ts (1)
playNotificationSound(24-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| import { productName } from "~/package.json"; | ||
| import { setMainWindow } from "../lib/auto-updater"; | ||
| import { createApplicationMenu } from "../lib/menu"; | ||
| import { playNotificationSound } from "../lib/notification-sound"; |
There was a problem hiding this comment.
Use TS path alias for notification-sound import and fix Linux fallback typo
- To match the existing alias usage in this file and the desktop TS guideline, consider importing via the alias (if configured) instead of a relative path, e.g.
import { playNotificationSound } from "lib/notification-sound";for consistency. - Related: in
apps/desktop/src/main/lib/notification-sound.ts(lines 23–46), the Linux fallback usesexecFile("paplay", ...)and thenexecFile("atray", ...)if that fails.atraylooks like a typo foraplay, so on systems withoutpaplaythe fallback will never succeed, and withsilent: truehere the user may get no sound at all.
🤖 Prompt for AI Agents
In apps/desktop/src/main/windows/main.ts around line 11, change the relative
import to use the TS path alias so it reads import { playNotificationSound }
from "lib/notification-sound"; for consistency; also in
apps/desktop/src/main/lib/notification-sound.ts (approx lines 23–46) fix the
Linux fallback by replacing the incorrect "atray" with "aplay" and stop
suppressing errors/sound by removing or disabling silent:true (or set
silent:false) and surface/log any execFile errors so the fallback can actually
play audio and failures are visible.
1 participant
Summary
notification-sound.tsutility that plays sounds cross-platform (macOS, Windows, Linux)notification.mp3with fade-out effect in resourcesTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.