feat (desktop): toggle for notifications#1045
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds notification-sounds muting: DB migration and schema column, TRPC procedures to get/set muted state, settings UI toggle with optimistic updates, removal of the "none" ringtone entry, and playback gating so notifications skip sound when muted. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Settings UI
participant TRPC as TRPC Server
participant DB as LocalDB
participant Notifier as Notification System
User->>UI: toggle mute switch
UI->>UI: optimistic UI update
UI->>TRPC: setNotificationSoundsMuted({ muted })
TRPC->>DB: upsert settings.notification_sounds_muted
DB-->>TRPC: confirmation
TRPC-->>UI: { success: true }
Note over UI: ringtone grid/tips hidden when muted
Notifier->>DB: areNotificationSoundsMuted()
DB-->>Notifier: boolean
alt muted
Notifier->>Notifier: skip playback
else not muted
Notifier->>Notifier: play sound file
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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 (2)
168-170: Don't silently swallow cleanup errors.The
catch(() => {})pattern violates the coding guideline to never silently swallow errors. Even in cleanup scenarios, errors should be logged at minimum for debugging purposes.Proposed fix
electronTrpcClient.ringtone.stop.mutate().catch(() => { - // Ignore errors during cleanup + (err) => { + console.error("[ringtone/cleanup] Failed to stop preview on unmount:", err); + } });Actually, the fix should be:
- electronTrpcClient.ringtone.stop.mutate().catch(() => { - // Ignore errors during cleanup - }); + electronTrpcClient.ringtone.stop.mutate().catch((err) => { + console.error("[ringtone/cleanup] Failed to stop preview on unmount:", err); + });
266-283: Stop ringtone preview when muting is toggled.When a user starts previewing a ringtone and then toggles the mute switch, the sound continues playing but the UI disappears. Add a
useEffectto stop the preview and clearplayingIdwhenisMutedchanges:useEffect(() => { if (isMuted && playingId) { electronTrpcClient.ringtone.stop.mutate().catch(() => { console.error("Failed to stop ringtone on mute"); }); setPlayingId(null); } }, [isMuted, playingId]);
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx`:
- Around line 146-152: The onError handler for the mutation currently swallows
errors; update the onError in RingtonesSettings (the function using onError:
(_err, _vars, context) => { ... }) to log the error and context before or after
restoring state with
utils.settings.getNotificationSoundsMuted.setData(undefined, context.previous);
include the _err and context (or context.previous) in the log call (e.g.,
console.error or the app logger) so the failure is recorded with relevant
context.
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/notification-sound.ts (1)
14-21: Add logging for errors instead of silently swallowing.The catch block silently returns
falsewithout logging the error. This makes debugging harder when database access fails unexpectedly. As per coding guidelines, errors should at minimum be logged with context.♻️ Suggested fix
function areNotificationSoundsMuted(): boolean { try { const settingsRow = localDb.select().from(settings).get(); return settingsRow?.notificationSoundsMuted ?? false; - } catch { + } catch (error) { + console.warn("[notification-sound/areNotificationSoundsMuted] Failed to read muted state:", error); return false; } }apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx (1)
188-221: Console logging missing domain prefix and magic numbers.Per coding guidelines:
- Console logging should use
[domain/operation] messagepattern- Magic numbers should be extracted to named constants
Proposed fix
+const PLAYBACK_END_BUFFER_SECONDS = 0.5; +const MS_PER_SECOND = 1000; + // ... in handleTogglePlay: } catch (error) { - console.error("Failed to stop ringtone:", error); + console.error("[ringtone/preview] Failed to stop ringtone:", error); } // ... and: } catch (error) { - console.error("Failed to play ringtone:", error); + console.error("[ringtone/preview] Failed to play ringtone:", error); setPlayingId(null); } // Auto-reset after the ringtone's actual duration (with buffer) - const durationMs = ((ringtone.duration ?? 5) + 0.5) * 1000; + const durationMs = ((ringtone.duration ?? 5) + PLAYBACK_END_BUFFER_SECONDS) * MS_PER_SECOND;
Comment on lines
+146
to
+152
| onError: (_err, _vars, context) => { | ||
| if (context?.previous !== undefined) { | ||
| utils.settings.getNotificationSoundsMuted.setData( | ||
| undefined, | ||
| context.previous, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Log the error in onError handler.
The error is being silently handled without logging. Per coding guidelines, errors should at minimum be logged with context.
Proposed fix
- onError: (_err, _vars, context) => {
+ onError: (err, _vars, context) => {
+ console.error("[settings/notification-sounds] Failed to toggle muted state:", err);
if (context?.previous !== undefined) {
utils.settings.getNotificationSoundsMuted.setData(
undefined,
context.previous,
);
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onError: (_err, _vars, context) => { | |
| if (context?.previous !== undefined) { | |
| utils.settings.getNotificationSoundsMuted.setData( | |
| undefined, | |
| context.previous, | |
| ); | |
| } | |
| onError: (err, _vars, context) => { | |
| console.error("[settings/notification-sounds] Failed to toggle muted state:", err); | |
| if (context?.previous !== undefined) { | |
| utils.settings.getNotificationSoundsMuted.setData( | |
| undefined, | |
| context.previous, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx`
around lines 146 - 152, The onError handler for the mutation currently swallows
errors; update the onError in RingtonesSettings (the function using onError:
(_err, _vars, context) => { ... }) to log the error and context before or after
restoring state with
utils.settings.getNotificationSoundsMuted.setData(undefined, context.previous);
include the _err and context (or context.previous) in the log call (e.g.,
console.error or the app logger) so the failure is recorded with relevant
context.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/notification-sound.ts`:
- Around line 14-21: The catch block in areNotificationSoundsMuted currently
swallows errors; update it to log the caught error (include the error object and
context indicating the function and that a default false will be returned)
before returning false—use the project's logger if available (e.g.,
appLogger.error or similar), otherwise console.error; reference localDb and
settings in the log message so the failure source is clear.
Comment on lines
+14
to
+21
| function areNotificationSoundsMuted(): boolean { | ||
| try { | ||
| const settingsRow = localDb.select().from(settings).get(); | ||
| return settingsRow?.notificationSoundsMuted ?? false; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Log the error before returning the default value.
The catch block silently swallows errors. Per coding guidelines, errors should at minimum be logged with context, even when providing a safe fallback.
Proposed fix
function areNotificationSoundsMuted(): boolean {
try {
const settingsRow = localDb.select().from(settings).get();
return settingsRow?.notificationSoundsMuted ?? false;
- } catch {
+ } catch (error) {
+ console.error("[notification-sound/areNotificationSoundsMuted] Failed to read muted state:", error);
return false;
}
}🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/notification-sound.ts` around lines 14 - 21, The
catch block in areNotificationSoundsMuted currently swallows errors; update it
to log the caught error (include the error object and context indicating the
function and that a default false will be returned) before returning false—use
the project's logger if available (e.g., appLogger.error or similar), otherwise
console.error; reference localDb and settings in the log message so the failure
source is clear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.