feat(desktop): allow custom notification audio files#1705
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes implement a custom ringtone feature for the desktop app by refactoring the ringtone system from filename-based to ID-based resolution. A new backend module manages custom audio file validation and storage, TRPC routers expose import/retrieval endpoints, and frontend components enable users to import custom audio files. Persistent state now uses backend-driven JSON storage via TRPC. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as RingtonesSettings<br/>(UI Component)
participant Dialog as Electron<br/>Dialog
participant TRPC as Ringtone<br/>TRPC Router
participant Backend as custom-ringtones<br/>Module
participant FS as File System
User->>UI: Clicks "Add Custom Audio"
UI->>Dialog: Opens file selection
Dialog->>User: Shows picker
User->>Dialog: Selects audio file
Dialog->>TRPC: importCustom()
TRPC->>Backend: importCustomRingtoneFromPath(sourcePath)
Backend->>FS: Validate file (type, size)
Backend->>FS: Create custom ringtones dir
Backend->>FS: Copy to temp, atomic replace
Backend->>FS: Write metadata JSON
Backend->>Backend: Return CustomRingtoneInfo
TRPC->>TRPC: Return success
UI->>TRPC: Invalidate getCustom cache
UI->>TRPC: Set selectedRingtoneId = "custom"
UI->>User: Display custom ringtone in list
sequenceDiagram
participant Store as Ringtone<br/>Store (Zustand)
participant TRPC as TRPC<br/>Client
participant Router as Ringtone<br/>Router
participant Backend as Backend<br/>Helpers
participant Session as Session<br/>Playback
Store->>TRPC: Preview ringtoneId
TRPC->>Router: preview.mutate({ ringtoneId })
Router->>Backend: getRingtoneSoundPath(ringtoneId)
alt Built-in ID
Backend->>Backend: getSoundPath(filename)
else Custom ID
Backend->>Backend: getCustomRingtonePath()
else Invalid
Backend->>Backend: Return default path
end
Backend->>Router: Return soundPath
Router->>Session: Play sound from path
Session->>Session: Audio playback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx (1)
240-245:⚠️ Potential issue | 🟡 MinorPreview timer won't match custom ringtone duration.
Custom ringtones don't carry a
durationfield, so the timer defaults to5.5s. A user-imported audio file could be much longer (up to 20MB), causing the UI to show the ringtone as "stopped" while it's still playing. This is minor since it only affects the visual indicator, not actual playback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx` around lines 240 - 245, The preview timer uses ringtone.duration which is missing for custom imports, so update the play preview flow in RingtonesSettings to determine duration from the audio source instead of ringtone.duration: create or reuse an Audio element (or use the existing player) and wait for its loadedmetadata/duration value, then compute durationMs from audio.duration (with the 500ms buffer) and set previewTimerRef.current = setTimeout(...) to call setPlayingId and clear previewTimerRef as before; keep the old fallback (e.g., 5s) if duration is NaN or unavailable and ensure any existing previewTimerRef is cleared before creating a new timer.apps/desktop/src/main/lib/notification-sound.ts (1)
64-69:⚠️ Potential issue | 🟡 MinorPowerShell command may break for paths containing single quotes.
The
soundPathis interpolated into a single-quoted PowerShell string. If the user's home directory contains a'(e.g.,C:\Users\O'Brien\), the command breaks silently. This is pre-existing but now more relevant since custom ringtone paths go throughhomedir().Consider escaping the path or using double quotes with proper escaping:
Proposed fix
- execFile("powershell", [ - "-c", - `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`, - ]); + const escapedPath = soundPath.replace(/'/g, "''"); + execFile("powershell", [ + "-c", + `(New-Object Media.SoundPlayer '${escapedPath}').PlaySync()`, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/notification-sound.ts` around lines 64 - 69, The PowerShell command embeds soundPath inside a single-quoted string which breaks if the path contains a single quote; fix this in the Windows branch where execFile("powershell", ["-c", `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`]) is used by escaping single quotes in soundPath before interpolation (e.g., compute const safePath = soundPath.replace(/'/g, "''") and use safePath in the interpolated command) so paths like O'Brien won't break.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx (1)
168-179: No user-facing feedback on import failure.
onErroronly logs to console. If the import fails (e.g., file too large, unsupported format), the user has no indication that something went wrong. Consider showing a toast or inline error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx` around lines 168 - 179, The onError handler for the importCustomRingtone mutation only logs to the console so users get no feedback; update importCustomRingtone.onError to surface a user-facing error (e.g., call your toast notification or set an inline error state) and include the error.message/details, then ensure the UI clears or updates appropriately (you may reuse the existing state setters like setRingtone or add a new error state and show it in RingtonesSettings UI); keep the existing console.error but also call the app's toast/showError function (or set a local error state) so failures like "file too large" or "unsupported format" are visible to the user and do not silently fail.apps/desktop/src/renderer/stores/ringtone/store.ts (1)
3-8: Use path aliasshared/ringtonesinstead of relative path.Other renderer files in this PR (e.g.,
RingtonesSettings.tsxline 15) import from"shared/ringtones", but here it's"../../../shared/ringtones". As per coding guidelines, "Use alias as defined intsconfig.jsonwhen possible."Proposed fix
import { CUSTOM_RINGTONE_ID, DEFAULT_RINGTONE_ID, RINGTONES, type RingtoneData, -} from "../../../shared/ringtones"; +} from "shared/ringtones";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/ringtone/store.ts` around lines 3 - 8, Replace the relative import at the top of the file with the path alias import used elsewhere: import CUSTOM_RINGTONE_ID, DEFAULT_RINGTONE_ID, RINGTONES and the RingtoneData type from "shared/ringtones" instead of "../../../shared/ringtones"; update the import statement that currently pulls CUSTOM_RINGTONE_ID, DEFAULT_RINGTONE_ID, RINGTONES and type RingtoneData so it uses the alias to match other renderer files.apps/desktop/src/lib/trpc/routers/ringtone/index.ts (1)
49-99: Code duplication:playSoundFileimplementations across two files serve different architectural purposes.Two separate implementations exist:
apps/desktop/src/main/lib/notification-sound.ts: Fire-and-forget playback (used by system notification flow via IPC)apps/desktop/src/lib/trpc/routers/ringtone/index.ts: Session-tracked playback (supports stop/preview control in ringtone router)The duplication is intentional due to differing requirements: the ringtone router needs stop capability and race-condition handling via session tracking, while the notification system requires simple fire-and-forget behavior. Consider extracting platform-specific audio command logic into a shared utility while preserving separate wrappers for these different use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts` around lines 49 - 99, There is duplicated platform-specific audio-command logic between the session-tracked playSoundFile in the ringtone router and the fire-and-forget playback in apps/desktop/src/main/lib/notification-sound.ts; extract the shared platform/command resolution and fallback strategy into a single utility (e.g., resolveAudioPlayerCommand or createAudioPlayerExecutor) that returns the appropriate execFile invocation details or a small helper to invoke a command, then have playSoundFile (session-tracked) and the notification sound wrapper call that utility: keep playSoundFile's session/stop/race handling and keep the notification-sound's fire-and-forget behavior, but delegate the platform-specific command choices (darwin: afplay, win32: powershell SoundPlayer, linux: paplay->aplay fallback) to the shared helper referenced from both places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts`:
- Around line 202-209: The exported function playNotificationRingtone is dead
code and should be removed; delete the entire playNotificationRingtone
function/export (including its internal calls to getRingtoneSoundPath and
playSoundFile) from this module, and if those helper imports
(getRingtoneSoundPath, playSoundFile) become unused after removal, remove their
import statements as well; if this function was intended for external use
instead of the existing playNotificationSound workflow, replace it with a clear,
documented wrapper that delegates to the canonical playNotificationSound
implementation rather than keeping this unused duplicate.
In `@apps/desktop/src/main/lib/custom-ringtones.ts`:
- Around line 184-193: The current flow deletes old ringtones via
removeExistingCustomRingtoneFiles() before awaiting copyFile(), which can
permanently lose the previous file on copy failure; change to perform an atomic
replace: use ensureCustomRingtonesDir(), copy the sourcePath to a temporary
target inside RINGTONES_ASSETS_DIR (e.g., CUSTOM_RINGTONE_FILE_STEM + ".tmp" +
ext), await that copy, then call removeExistingCustomRingtoneFiles() (or unlink
the old file) and finally rename the temp file to the final name
(CUSTOM_RINGTONE_FILE_STEM + ext); alternatively, if you keep the current steps,
at minimum wrap copyFile(...) in try/catch and do not remove the old file unless
copy succeeds, logging the error on failure (reference functions:
ensureCustomRingtonesDir, removeExistingCustomRingtoneFiles, copyFile, and
constants CUSTOM_RINGTONE_FILE_STEM / RINGTONES_ASSETS_DIR).
---
Outside diff comments:
In `@apps/desktop/src/main/lib/notification-sound.ts`:
- Around line 64-69: The PowerShell command embeds soundPath inside a
single-quoted string which breaks if the path contains a single quote; fix this
in the Windows branch where execFile("powershell", ["-c", `(New-Object
Media.SoundPlayer '${soundPath}').PlaySync()`]) is used by escaping single
quotes in soundPath before interpolation (e.g., compute const safePath =
soundPath.replace(/'/g, "''") and use safePath in the interpolated command) so
paths like O'Brien won't break.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx`:
- Around line 240-245: The preview timer uses ringtone.duration which is missing
for custom imports, so update the play preview flow in RingtonesSettings to
determine duration from the audio source instead of ringtone.duration: create or
reuse an Audio element (or use the existing player) and wait for its
loadedmetadata/duration value, then compute durationMs from audio.duration (with
the 500ms buffer) and set previewTimerRef.current = setTimeout(...) to call
setPlayingId and clear previewTimerRef as before; keep the old fallback (e.g.,
5s) if duration is NaN or unavailable and ensure any existing previewTimerRef is
cleared before creating a new timer.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts`:
- Around line 49-99: There is duplicated platform-specific audio-command logic
between the session-tracked playSoundFile in the ringtone router and the
fire-and-forget playback in apps/desktop/src/main/lib/notification-sound.ts;
extract the shared platform/command resolution and fallback strategy into a
single utility (e.g., resolveAudioPlayerCommand or createAudioPlayerExecutor)
that returns the appropriate execFile invocation details or a small helper to
invoke a command, then have playSoundFile (session-tracked) and the notification
sound wrapper call that utility: keep playSoundFile's session/stop/race handling
and keep the notification-sound's fire-and-forget behavior, but delegate the
platform-specific command choices (darwin: afplay, win32: powershell
SoundPlayer, linux: paplay->aplay fallback) to the shared helper referenced from
both places.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx`:
- Around line 168-179: The onError handler for the importCustomRingtone mutation
only logs to the console so users get no feedback; update
importCustomRingtone.onError to surface a user-facing error (e.g., call your
toast notification or set an inline error state) and include the
error.message/details, then ensure the UI clears or updates appropriately (you
may reuse the existing state setters like setRingtone or add a new error state
and show it in RingtonesSettings UI); keep the existing console.error but also
call the app's toast/showError function (or set a local error state) so failures
like "file too large" or "unsupported format" are visible to the user and do not
silently fail.
In `@apps/desktop/src/renderer/stores/ringtone/store.ts`:
- Around line 3-8: Replace the relative import at the top of the file with the
path alias import used elsewhere: import CUSTOM_RINGTONE_ID,
DEFAULT_RINGTONE_ID, RINGTONES and the RingtoneData type from "shared/ringtones"
instead of "../../../shared/ringtones"; update the import statement that
currently pulls CUSTOM_RINGTONE_ID, DEFAULT_RINGTONE_ID, RINGTONES and type
RingtoneData so it uses the alias to match other renderer files.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/ringtone/index.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/lib/custom-ringtones.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/shared/ringtones.ts
| export function playNotificationRingtone(ringtoneId: string): void { | ||
| const soundPath = getRingtoneSoundPath(ringtoneId); | ||
| if (!soundPath) { | ||
| return; | ||
| } | ||
|
|
||
| const soundPath = getSoundPath(filename); | ||
| playSoundFile(soundPath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of playNotificationRingtone
echo "=== Usages of playNotificationRingtone ==="
rg -n 'playNotificationRingtone' --type=ts --type=tsx -C2
echo ""
echo "=== Usages of playNotificationSound ==="
# Also check playNotificationSound usages
rg -n 'playNotificationSound' --type=ts --type=tsx -C2Repository: superset-sh/superset
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Find all usages of playNotificationRingtone - search all ts/tsx files
echo "=== Usages of playNotificationRingtone ==="
rg 'playNotificationRingtone' -t ts -C2
echo ""
echo "=== Usages of playNotificationSound ==="
rg 'playNotificationSound' -t ts -C2
echo ""
echo "=== Files matching ringtone related patterns ==="
find . -name "*ringtone*" -o -name "*notification*sound*"Repository: superset-sh/superset
Length of output: 2307
Remove playNotificationRingtone - it is dead code.
The function at lines 202-209 is never called anywhere in the codebase despite being exported. The notification system uses playNotificationSound from apps/desktop/src/main/lib/notification-sound.ts instead (imported and passed to NotificationManager in apps/desktop/src/main/windows/main.ts). The inline comment claiming this function "is used by the notification system" is incorrect and misleading. Either remove this unused export or clarify its purpose if it is intended for external use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts` around lines 202 - 209,
The exported function playNotificationRingtone is dead code and should be
removed; delete the entire playNotificationRingtone function/export (including
its internal calls to getRingtoneSoundPath and playSoundFile) from this module,
and if those helper imports (getRingtoneSoundPath, playSoundFile) become unused
after removal, remove their import statements as well; if this function was
intended for external use instead of the existing playNotificationSound
workflow, replace it with a clear, documented wrapper that delegates to the
canonical playNotificationSound implementation rather than keeping this unused
duplicate.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/lib/trpc-storage.ts (2)
53-56: Unhandled throw fromonSetErrorwould escape the catch block.If a future
onSetErrorimplementation throws, the exception escapessetItem's catch block as an unhandled rejection. The current ringtone handler is safe (it has its own try/catch), but the general contract is fragile.🛡️ Suggested defensive wrap
} catch (error) { console.error("[trpc-storage] Failed to set state:", error); - await config.onSetError?.(error); + try { + await config.onSetError?.(error); + } catch (callbackError) { + console.error("[trpc-storage] onSetError callback failed:", callbackError); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/trpc-storage.ts` around lines 53 - 56, In setItem's catch block, avoid letting exceptions from config.onSetError escape by calling await config.onSetError?.(error) inside its own try/catch (or otherwise safely handling/re-logging any errors it throws); update the catch that currently logs "[trpc-storage] Failed to set state:" so it then wraps the onSetError invocation in a defensive try/catch and logs any secondary errors so setItem never lets onSetError rethrow; reference the setItem catch block, the error local, and config.onSetError.
62-67: Logic order issue ingetSelectedRingtone: custom-ringtone check comes after thefindlookup.Lines 64-66 search
AVAILABLE_RINGTONESfor the selected ID, but the result is never used when the ID isCUSTOM_RINGTONE_ID. Thefindon line 64 will always returnundefinedfor a custom ringtone since it's not in the built-in list, making it wasted work. More importantly, the check on line 67 should come before thefindto make the intent clear and avoid falling through to the stale/invalid branch on line 72 if the order were ever rearranged.♻️ Suggested reorder
getSelectedRingtone: () => { const state = get(); + if (state.selectedRingtoneId === CUSTOM_RINGTONE_ID) { + // Custom ringtones are resolved by backend file state, not the built-in list. + return getDefaultRingtone(); + } const ringtone = AVAILABLE_RINGTONES.find( (r) => r.id === state.selectedRingtoneId, ); - if (state.selectedRingtoneId === CUSTOM_RINGTONE_ID) { - // Custom ringtones are resolved by backend file state, not the built-in list. - return getDefaultRingtone(); - } // Fall back to default if persisted ID is stale/invalid if (!ringtone) { set({ selectedRingtoneId: DEFAULT_RINGTONE_ID }); return getDefaultRingtone(); } return ringtone; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/trpc-storage.ts` around lines 62 - 67, Reorder the logic in getSelectedRingtone so the CUSTOM_RINGTONE_ID check runs before searching AVAILABLE_RINGTONES: first test if selectedId === CUSTOM_RINGTONE_ID and return the custom ringtone payload, otherwise run the find against AVAILABLE_RINGTONES to locate a built-in ringtone; this avoids an unnecessary find for the custom case and prevents falling through to the stale/invalid branch when the id is the custom constant.apps/desktop/src/renderer/stores/ringtone/store.ts (1)
62-77: Logic order:findresult is computed but unused when ID isCUSTOM_RINGTONE_ID.The
AVAILABLE_RINGTONES.find(...)on line 64 will always returnundefinedforCUSTOM_RINGTONE_ID(it's not in the built-in list). Moving the custom-ID check before thefindavoids the wasted lookup and makes the control flow clearer.♻️ Suggested reorder
getSelectedRingtone: () => { const state = get(); + if (state.selectedRingtoneId === CUSTOM_RINGTONE_ID) { + // Custom ringtones are resolved by backend file state, not the built-in list. + return getDefaultRingtone(); + } const ringtone = AVAILABLE_RINGTONES.find( (r) => r.id === state.selectedRingtoneId, ); - if (state.selectedRingtoneId === CUSTOM_RINGTONE_ID) { - // Custom ringtones are resolved by backend file state, not the built-in list. - return getDefaultRingtone(); - } // Fall back to default if persisted ID is stale/invalid if (!ringtone) { set({ selectedRingtoneId: DEFAULT_RINGTONE_ID });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/ringtone/store.ts` around lines 62 - 77, In getSelectedRingtone, the AVAILABLE_RINGTONES.find call is performed even when state.selectedRingtoneId === CUSTOM_RINGTONE_ID (which will never be in the built-in list); move the custom-ID check (CUSTOM_RINGTONE_ID) to occur before computing ringtone so you skip the find for custom ringtones, and keep the fallback logic that sets selectedRingtoneId to DEFAULT_RINGTONE_ID and returns getDefaultRingtone() when find returns undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/lib/trpc-storage.ts`:
- Around line 53-56: In setItem's catch block, avoid letting exceptions from
config.onSetError escape by calling await config.onSetError?.(error) inside its
own try/catch (or otherwise safely handling/re-logging any errors it throws);
update the catch that currently logs "[trpc-storage] Failed to set state:" so it
then wraps the onSetError invocation in a defensive try/catch and logs any
secondary errors so setItem never lets onSetError rethrow; reference the setItem
catch block, the error local, and config.onSetError.
- Around line 62-67: Reorder the logic in getSelectedRingtone so the
CUSTOM_RINGTONE_ID check runs before searching AVAILABLE_RINGTONES: first test
if selectedId === CUSTOM_RINGTONE_ID and return the custom ringtone payload,
otherwise run the find against AVAILABLE_RINGTONES to locate a built-in
ringtone; this avoids an unnecessary find for the custom case and prevents
falling through to the stale/invalid branch when the id is the custom constant.
In `@apps/desktop/src/renderer/stores/ringtone/store.ts`:
- Around line 62-77: In getSelectedRingtone, the AVAILABLE_RINGTONES.find call
is performed even when state.selectedRingtoneId === CUSTOM_RINGTONE_ID (which
will never be in the built-in list); move the custom-ID check
(CUSTOM_RINGTONE_ID) to occur before computing ringtone so you skip the find for
custom ringtones, and keep the fallback logic that sets selectedRingtoneId to
DEFAULT_RINGTONE_ID and returns getDefaultRingtone() when find returns
undefined.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/main/lib/custom-ringtones.tsapps/desktop/src/renderer/lib/trpc-storage.tsapps/desktop/src/renderer/stores/ringtone/store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/custom-ringtones.ts
Summary
Changes
custom-ringtonesmain utility for secure storage/import/lookupValidation
bun run --cwd apps/desktop typecheckSummary by CodeRabbit