feat(desktop): improve ringtone settings with duration display and bug fixes#318
feat(desktop): improve ringtone settings with duration display and bug fixes#318
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request introduces a complete ringtone management system for the desktop application, including IPC handlers for preview, stop, and list operations; a Zustand-based state store with tRPC-backed persistence; a RingtonesSettings UI component; and updates to the notification sound flow to use dynamically selected ringtones. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as RingtonesSettings UI
participant Store as Zustand Store
participant tRPC as tRPC Client
participant Server as tRPC Server
participant DB as Database
participant IPC as IPC Handler
rect rgb(200, 220, 255)
note over User,DB: Ringtone Selection Flow
User->>UI: Click to select ringtone
UI->>Store: setRingtone(ringtoneId)
Store->>tRPC: setSelectedRingtoneId.mutate({ringtoneId})
tRPC->>Server: Validate & persist ringtoneId
Server->>DB: Save selectedRingtoneId to Settings
DB-->>Server: Success
Server-->>tRPC: Return confirmation
tRPC-->>Store: Rehydrate state
Store-->>UI: State updated
end
rect rgb(220, 255, 220)
note over User,IPC: Ringtone Preview Flow
User->>UI: Click preview button on card
UI->>IPC: ringtone:preview({filename})
IPC->>IPC: Resolve ringtone path
IPC->>IPC: Spawn child process (afplay/PowerShell/paplay)
IPC-->>UI: {success: true}
UI->>UI: Set play state, start auto-reset timer
Note over IPC: Audio plays in background
IPC->>IPC: Track session, clear on completion
UI->>IPC: [After duration] ringtone:stop()
IPC-->>UI: {success: true}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (1)
6-21: Ringtones section wiring is correct, but RingtonesSettings should use tRPC-based IPCThe new
"ringtones"section is correctly integrated (type-safeSettingsSection, matching sidebar id, and conditional render of<RingtonesSettings />). However,RingtonesSettingsshould use the tRPC layer defined insrc/lib/trpcfor all interprocess communication instead of directipcRenderer.invoke()calls. Per the desktop coding guidelines, all IPC inapps/desktop/**/*.{ts,tsx,js,jsx}must be routed through tRPC.apps/desktop/src/main/lib/notification-sound.ts (2)
75-89: Missing error handling for macOS and Windows playback.The
execFilecalls for macOS (line 76) and Windows (lines 78-81) don't handle errors, unlike the Linux path which has a fallback. Failed playback will silently fail without any logging.if (process.platform === "darwin") { - execFile("afplay", [soundPath]); + execFile("afplay", [soundPath], (error) => { + if (error) { + console.warn("[notification-sound] Failed to play sound:", error); + } + }); } else if (process.platform === "win32") { execFile("powershell", [ "-c", `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`, - ]); + ], (error) => { + if (error) { + console.warn("[notification-sound] Failed to play sound:", error); + } + }); } else {
78-81: Potential command injection via soundPath in PowerShell.The
soundPathis interpolated directly into the PowerShell command string. If a filename contained single quotes or special characters, it could break execution or potentially allow injection. Consider using proper escaping or a safer approach.} else if (process.platform === "win32") { + // Escape single quotes in path for PowerShell + const escapedPath = soundPath.replace(/'/g, "''"); execFile("powershell", [ "-c", - `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`, + `(New-Object Media.SoundPlayer '${escapedPath}').PlaySync()`, ]);Alternatively, consider using a node-native audio library that doesn't require shell commands.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/lib/electron-storage.ts (1)
7-18: Consider adding explicit typing for the storage adapter.The adapter works correctly but uses implicit typing. Consider explicitly typing it as
StateStoragefromzustand/middlewarefor better type safety and IDE support.+import { createJSONStorage, type StateStorage } from "zustand/middleware"; -import { createJSONStorage } from "zustand/middleware"; -const electronStorageAdapter = { +const electronStorageAdapter: StateStorage = {apps/desktop/src/shared/ipc-channels/storage.ts (1)
5-19: Consider usingIpcResponsewrapper for consistency.Unlike
RingtoneChannelswhich usesIpcResponsefor responses,StorageChannelsuses raw types (unknown,undefined). This inconsistency could complicate error handling on the renderer side. Consider aligning with the pattern used in other channel definitions.+import type { IpcResponse } from "./types"; + export interface StorageChannels { "storage:get": { request: { key: string }; - response: unknown; + response: IpcResponse<unknown>; }; "storage:set": { request: { key: string; value: unknown }; - response: undefined; + response: IpcResponse; }; "storage:delete": { request: { key: string }; - response: undefined; + response: IpcResponse; }; }apps/desktop/src/renderer/stores/ringtone/store.ts (1)
41-46:getSelectedRingtonemethod duplicatesuseSelectedRingtonehook logic.The
getSelectedRingtonemethod on the store and theuseSelectedRingtonehook (lines 65-67) contain identical logic. This creates maintenance burden and potential drift.Consider removing the method and using the hook exclusively, or extracting a shared selector:
+// Shared selector for reuse +const selectRingtone = (state: RingtoneState) => + AVAILABLE_RINGTONES.find((r) => r.id === state.selectedRingtoneId); + export const useRingtoneStore = create<RingtoneState>()( // ... rest of store ); -export const useSelectedRingtone = () => - useRingtoneStore((state) => - AVAILABLE_RINGTONES.find((r) => r.id === state.selectedRingtoneId), - ); +export const useSelectedRingtone = () => useRingtoneStore(selectRingtone);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
apps/desktop/src/resources/sounds/agentisdonewoman.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteafrican.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteafrobeat.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteedm.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/comebacktothecode.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/shabalabadingdong.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/supersetdoowap.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/supersetquick.mp3is excluded by!**/*.mp3bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/notification-sound.ts(2 hunks)apps/desktop/src/main/lib/ringtone-ipcs.ts(1 hunks)apps/desktop/src/main/lib/storage-ipcs.ts(1 hunks)apps/desktop/src/main/lib/storage-manager.ts(1 hunks)apps/desktop/src/preload/index.ts(2 hunks)apps/desktop/src/renderer/lib/electron-storage.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx(2 hunks)apps/desktop/src/renderer/stores/app-state.ts(1 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/renderer/stores/ringtone/index.ts(1 hunks)apps/desktop/src/renderer/stores/ringtone/store.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(2 hunks)apps/desktop/src/shared/ipc-channels/ringtone.ts(1 hunks)apps/desktop/src/shared/ipc-channels/storage.ts(1 hunks)apps/desktop/src/shared/ringtones.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/shared/ipc-channels/ringtone.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/main/lib/storage-manager.tsapps/desktop/src/renderer/stores/index.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/shared/ringtones.tsapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/renderer/stores/ringtone/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/shared/ipc-channels/storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
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/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/shared/ipc-channels/ringtone.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/main/lib/storage-manager.tsapps/desktop/src/renderer/stores/index.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/shared/ringtones.tsapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/renderer/stores/ringtone/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/shared/ipc-channels/storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/shared/ipc-channels/ringtone.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/main/lib/storage-manager.tsapps/desktop/src/renderer/stores/index.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/shared/ringtones.tsapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/renderer/stores/ringtone/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/shared/ipc-channels/storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - only in src/main/
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/renderer/stores/index.tsapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/stores/ringtone/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env file in src/main/index.ts with override: true
Files:
apps/desktop/src/main/index.ts
**/components/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one folder per component with structure ComponentName/ComponentName.tsx + index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
apps/desktop/src/main/lib/*-ipcs.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system with handlers accepting single object parameters, not positional parameters
Files:
apps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/main/lib/ringtone-ipcs.ts
🧠 Learnings (8)
📚 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/shared/ipc-channels/ringtone.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/preload/index.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/shared/ipc-channels/storage.ts
📚 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} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/lib/electron-storage.ts
📚 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/renderer/lib/electron-storage.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/preload/index.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/shared/ipc-channels/storage.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/main/lib/*{workspace,worktree}-*.ts : Use git worktree-based workspace management in the desktop app through workspace-manager.ts and worktree-manager.ts
Applied to files:
apps/desktop/src/main/lib/storage-manager.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/main/lib/ringtone-ipcs.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/renderer/stores/index.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/main/lib/ringtone-ipcs.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/renderer/stores/index.tsapps/desktop/src/main/lib/ringtone-ipcs.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/main/lib/*-ipcs.ts : Use type-safe IPC system with handlers accepting single object parameters, not positional parameters
Applied to files:
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/storage-ipcs.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/shared/ipc-channels/storage.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/main/lib/terminal-*.ts : Use node-pty for terminal session management in the desktop app
Applied to files:
apps/desktop/src/main/lib/ringtone-ipcs.ts
🧬 Code graph analysis (6)
apps/desktop/src/main/lib/storage-manager.ts (1)
apps/desktop/src/main/lib/app-environment.ts (1)
SUPERSET_HOME_DIR(8-8)
apps/desktop/src/shared/ringtones.ts (1)
apps/desktop/src/renderer/stores/ringtone/store.ts (1)
DEFAULT_RINGTONE_ID(13-13)
apps/desktop/src/renderer/stores/ringtone/store.ts (2)
apps/desktop/src/shared/ringtones.ts (3)
RingtoneData(6-15)RINGTONES(21-111)DEFAULT_RINGTONE_ID(113-113)apps/desktop/src/renderer/lib/electron-storage.ts (1)
electronStorage(20-20)
apps/desktop/src/main/lib/storage-ipcs.ts (1)
apps/desktop/src/main/lib/storage-manager.ts (1)
store(8-11)
apps/desktop/src/shared/ipc-channels/index.ts (3)
apps/desktop/src/shared/ipc-channels/ui.ts (1)
UiChannels(8-83)apps/desktop/src/shared/ipc-channels/storage.ts (1)
StorageChannels(5-20)apps/desktop/src/shared/ipc-channels/ringtone.ts (1)
RingtoneChannels(7-31)
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (1)
RingtonesSettings(151-266)apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.ts (1)
RingtonesSettings(1-1)
🪛 GitHub Actions: CI
apps/desktop/src/main/lib/storage-manager.ts
[error] 1-1: Rollup failed to resolve import 'electron-store'. This is likely due to a missing dependency or misconfigured build; consider adding 'electron-store' to dependencies or externalizing it in build.rollupOptions.external.
🔇 Additional comments (17)
apps/desktop/src/renderer/stores/index.ts (1)
7-9: Ringtone store barrel re-export fits existing patternRe-exporting
./ringtonealongside the other stores is consistent with this barrel and keeps imports ergonomic; no issues from a type or runtime perspective.apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx (1)
3-7: New “Ringtones” sidebar section is consistent and type-safeThe added
"ringtones"entry (withHiOutlineBellicon) cleanly matches theSettingsSectionunion and existing sidebar structure; no issues from a typing, UX, or layout perspective.Also applies to: 25-29
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.ts (1)
1-1: Barrel export for RingtonesSettings is appropriateRe-exporting
RingtonesSettingsfrom the folder index matches the component-folder convention and aligns with how it’s imported inSettingsContent.apps/desktop/src/main/index.ts (1)
8-9: Verify IPC handler registration timing and adherence to tRPC-based IPCWiring
registerStorageHandlers()andregisterRingtoneHandlers()at module top-level ensures IPC handlers are ready early, which is appropriate as long as those functions only register handlers (e.g., viaipcMain.handle) and don’t depend onapp.whenReady()or an existingBrowserWindow. Also, per the desktop IPC guidelines, these handlers should be implemented via the shared tRPC-based IPC layer rather than ad‑hoc channel handling.Please double-check that:
- The handler registration code does not rely on app readiness or window instances at import time.
- The storage and ringtone IPC modules are using the tRPC abstraction defined in
src/lib/trpcfor type-safe IPC.As per coding guidelines, IPC registration in apps/desktop should go through the project’s tRPC-based mechanism.
Also applies to: 33-35
apps/desktop/src/renderer/stores/ringtone/index.ts (1)
1-1: Ringtone store index export is straightforward and consistentRe-exporting everything from
"./store"keeps the ringtone store modular while exposing a clean public surface via the main stores barrel.apps/desktop/src/renderer/stores/app-state.ts (1)
5-11: ExtendingSettingsSectionwith “presets” and “ringtones” maintains type safetyAdding
"presets"and"ringtones"to theSettingsSectionunion cleanly aligns the store with the new settings sections used in the sidebar and content components, without impacting existing state logic.apps/desktop/src/shared/ipc-channels/ringtone.ts (1)
1-31: LGTM!The IPC channel definitions follow the established pattern with proper typing for request/response shapes and clear documentation. This aligns with the guideline to define IPC channel types before implementing handlers.
apps/desktop/src/shared/ipc-channels/index.ts (1)
11-12: LGTM!Clean integration of the new
StorageChannelsandRingtoneChannelsinto the combinedIpcChannelsinterface. The imports maintain alphabetical order and the interface extension follows the established pattern.Also applies to: 36-38
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (2)
74-91: Good accessibility pattern for nested interactive elements.Using
divwithrole="button"and proper keyboard handling (onKeyDown) to allow a nested play/stop button is a valid approach to avoid invalid HTML (button inside button). The biome suppression is appropriately documented.
157-168: Good cleanup implementation.The cleanup effect properly clears the timer and stops any playing sound when the component unmounts, preventing state updates after navigation. This addresses the PR objective for timer cleanup.
apps/desktop/src/main/lib/notification-sound.ts (1)
36-64: Good defensive parsing with proper fallbacks.The function handles multiple persistence formats (JSON string vs pre-parsed object), validates the ringtone ID, and falls back to defaults when data is missing or stale. This addresses the PR objective for fixing JSON parsing and handling unknown ringtone IDs.
apps/desktop/src/main/lib/ringtone-ipcs.ts (1)
123-147: LGTM!The
ringtone:listhandler correctly validates directory existence, filters by supported audio extensions, and returns a consistent response shape with proper error handling.apps/desktop/src/shared/ringtones.ts (3)
1-15: LGTM!Clean interface definition with appropriate optional
durationfield. The JSDoc comment clearly indicates the unit (seconds).
21-111: LGTM!Well-structured ringtone catalog with consistent metadata. The silent option correctly uses an empty filename, and the PR objectives confirm this is intentional for the "Silent" feature with visual distinction.
118-129: The fallback logic for unknown ringtone IDs is already implemented ingetSelectedRingtoneFilename(apps/desktop/src/main/lib/notification-sound.ts, lines 59–60), which returns the default ringtone filename if the selected one is empty. ThegetRingtoneFilenameutility function intentionally returns an empty string for unknown IDs to keep the function focused on its single responsibility, allowing callers to implement their own fallback logic as appropriate. The current design fulfills the PR objective.apps/desktop/src/renderer/stores/ringtone/store.ts (1)
60-68: LGTM!Clean convenience hooks that follow Zustand patterns. The selectors are properly scoped for optimal re-render behavior.
apps/desktop/src/preload/index.ts (1)
84-94: LGTM!The electronStore API is cleanly implemented with proper object parameter structure for IPC calls (
{ key },{ key, value }), consistent with the coding guideline for type-safe IPC handlers.
| function stopCurrentSound(): void { | ||
| if (currentPlayingProcess) { | ||
| // Use SIGKILL for immediate termination (afplay doesn't always respond to SIGTERM) | ||
| currentPlayingProcess.kill("SIGKILL"); | ||
| currentPlayingProcess = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
SIGKILL doesn't exist on Windows.
SIGKILL is a Unix signal and will not terminate processes on Windows. The kill() method on Windows requires SIGTERM or no argument (defaults to SIGTERM), and even then behavior differs.
Consider using a cross-platform approach:
function stopCurrentSound(): void {
if (currentPlayingProcess) {
- // Use SIGKILL for immediate termination (afplay doesn't always respond to SIGTERM)
- currentPlayingProcess.kill("SIGKILL");
+ // Use platform-appropriate signal for termination
+ if (process.platform === "win32") {
+ currentPlayingProcess.kill(); // Windows defaults to SIGTERM-like behavior
+ } else {
+ currentPlayingProcess.kill("SIGKILL"); // Unix: immediate termination
+ }
currentPlayingProcess = null;
}
}📝 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.
| function stopCurrentSound(): void { | |
| if (currentPlayingProcess) { | |
| // Use SIGKILL for immediate termination (afplay doesn't always respond to SIGTERM) | |
| currentPlayingProcess.kill("SIGKILL"); | |
| currentPlayingProcess = null; | |
| } | |
| } | |
| function stopCurrentSound(): void { | |
| if (currentPlayingProcess) { | |
| // Use platform-appropriate signal for termination | |
| if (process.platform === "win32") { | |
| currentPlayingProcess.kill(); // Windows defaults to SIGTERM-like behavior | |
| } else { | |
| currentPlayingProcess.kill("SIGKILL"); // Unix: immediate termination | |
| } | |
| currentPlayingProcess = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/ringtone-ipcs.ts around lines 38–44, the code calls
currentPlayingProcess.kill("SIGKILL") which is invalid on Windows; replace with
a cross-platform termination: if on win32, call taskkill via
child_process.spawnSync or use the tree-kill package to force-kill the PID
(e.g., treeKill(pid, 'SIGKILL') under the hood), otherwise use
currentPlayingProcess.kill('SIGKILL') for Unix; ensure you check
currentPlayingProcess.pid, handle errors, and set currentPlayingProcess = null
after successful termination.
| } else if (process.platform === "win32") { | ||
| currentPlayingProcess = execFile( | ||
| "powershell", | ||
| ["-c", `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`], | ||
| () => { | ||
| currentPlayingProcess = null; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Potential command injection via PowerShell.
The soundPath is interpolated directly into the PowerShell command string. If the path contains single quotes or special characters, it could break execution or allow injection.
Consider escaping the path or using a safer invocation pattern:
} else if (process.platform === "win32") {
+ // Escape single quotes in path for PowerShell
+ const escapedPath = soundPath.replace(/'/g, "''");
currentPlayingProcess = execFile(
"powershell",
- ["-c", `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`],
+ ["-c", `(New-Object Media.SoundPlayer '${escapedPath}').PlaySync()`],
() => {
currentPlayingProcess = null;
},
);📝 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.
| } else if (process.platform === "win32") { | |
| currentPlayingProcess = execFile( | |
| "powershell", | |
| ["-c", `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`], | |
| () => { | |
| currentPlayingProcess = null; | |
| }, | |
| ); | |
| } else if (process.platform === "win32") { | |
| // Escape single quotes in path for PowerShell | |
| const escapedPath = soundPath.replace(/'/g, "''"); | |
| currentPlayingProcess = execFile( | |
| "powershell", | |
| ["-c", `(New-Object Media.SoundPlayer '${escapedPath}').PlaySync()`], | |
| () => { | |
| currentPlayingProcess = null; | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/ringtone-ipcs.ts around lines 62 to 69, the
PowerShell command interpolates soundPath directly into a single-quoted command
which can be broken or exploited if the path contains quotes or special chars;
change to a safe invocation by passing the path as a separate argument (avoid
inline string interpolation) or properly escape/encode the path for PowerShell
(e.g., use parameterized invocation or --%/StopParsing where appropriate), and
ensure execFile receives the path as an argument array element rather than
concatenating it into the command string so special characters cannot cause
command injection; finally, preserve the existing callback behavior to clear
currentPlayingProcess.
| export function registerRingtoneHandlers() { | ||
| ipcMain.handle( | ||
| "ringtone:preview", | ||
| async (_event, input: { filename: string }) => { | ||
| try { | ||
| // Handle "none" case - no sound | ||
| if (!input.filename || input.filename === "") { | ||
| return { success: true }; | ||
| } | ||
|
|
||
| const soundPath = getRingtonePath(input.filename); | ||
| playSoundFile(soundPath); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| console.error("[ringtone:preview] Error:", error); | ||
| return { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }; | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other IPC handlers are implemented in the codebase
echo "=== Checking ipcMain.handle usage in main process ==="
rg -n "ipcMain.handle" apps/desktop/src/main --type ts -C 2
echo -e "\n=== Checking if tRPC is already used for IPC ==="
rg -n "trpc" apps/desktop/src --type ts | head -20Repository: superset-sh/superset
Length of output: 4695
🏁 Script executed:
#!/bin/bash
# Check the structure of src/lib/trpc
echo "=== Checking tRPC directory structure ==="
fd -t f "trpc" apps/desktop/src --type f
echo -e "\n=== Checking existing tRPC router patterns ==="
rg -n "router\(" apps/desktop/src/lib --type ts -A 5 | head -40Repository: superset-sh/superset
Length of output: 3151
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern of how IPC should be structured with tRPC
echo "=== Checking tRPC client/server setup ==="
cat apps/desktop/src/lib/trpc.ts 2>/dev/null || echo "File not found, checking for trpc-related files:"
fd "trpc" apps/desktop/src/lib --type fRepository: superset-sh/superset
Length of output: 154
Migrate to tRPC for IPC handlers.
This file uses raw ipcMain.handle but the established pattern in this codebase is to use tRPC (see apps/desktop/src/lib/trpc/routers/). Multiple handlers already use tRPC (window, settings, external, notifications), and per the coding guidelines, all Electron IPC must use tRPC. Create a tRPC router for ringtone operations and expose it alongside existing routers.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/ringtone-ipcs.ts around lines 87-108, the current
raw ipcMain.handle usage must be replaced by a tRPC router: create a new router
file (e.g. apps/desktop/src/lib/trpc/routers/ringtone.ts) that exposes a
"preview" procedure which accepts { filename: string } and implements the same
logic (handle empty/"none" filename, compute getRingtonePath, call
playSoundFile, return { success: true } or catch and return { success: false,
error: string }). Export the router and wire it into the app's main tRPC router
aggregator (the same module that currently registers
window/settings/external/notifications routers) so the ringtone procedures are
exposed via tRPC instead of ipcMain.handle; remove the old ipcMain.handle
registration from ringtone-ipcs.ts.
| import { ipcMain } from "electron"; | ||
| import { store } from "./storage-manager"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the storage-ipcs.ts file to understand current implementation
echo "=== Contents of storage-ipcs.ts ==="
cat -n apps/desktop/src/main/lib/storage-ipcs.ts
echo ""
echo "=== Checking for ipcMain.handle usage in main process ==="
rg -n "ipcMain\.handle" --type=ts apps/desktop/src/main/lib/ -A5
echo ""
echo "=== Checking for tRPC setup in main process ==="
fd -e ts apps/desktop/src/main/ | head -20 | xargs grep -l "trpc" 2>/dev/null || echo "No tRPC found in main process searches"
echo ""
echo "=== Checking src/lib/trpc structure ==="
ls -la apps/desktop/src/lib/trpc 2>/dev/null || echo "tRPC directory not found"Repository: superset-sh/superset
Length of output: 4104
🏁 Script executed:
#!/bin/bash
# Check tRPC router structure
echo "=== tRPC index.ts content ==="
cat -n apps/desktop/src/lib/trpc/index.ts
echo ""
echo "=== List of tRPC routers ==="
ls -la apps/desktop/src/lib/trpc/routers/
echo ""
echo "=== Check if storage router exists ==="
find apps/desktop/src/lib/trpc -name "*storage*" -type f
echo ""
echo "=== All IPC handler files in main/lib ==="
fd -e ts "ipcs\.ts$" apps/desktop/src/main/lib/
echo ""
echo "=== Check for any tRPC references in main process files ==="
rg -l "trpc" apps/desktop/src/main/ --type=tsRepository: superset-sh/superset
Length of output: 1825
Migrate storage IPC handlers to tRPC following the established pattern
This file uses raw ipcMain.handle() directly, which violates the coding guideline requiring all Electron IPC to use tRPC. A storage router should be created in apps/desktop/src/lib/trpc/routers/ and integrated following the pattern used by existing routers (config, ui-state, window, etc.), then these handlers should be migrated to tRPC procedures. The handlers are already type-safe with single object parameters, so only the integration pattern needs to change.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/storage-ipcs.ts lines 1-2, the file currently
registers raw ipcMain handlers which violates the rule that all Electron IPC
must go through tRPC; create a new storage router under
apps/desktop/src/lib/trpc/routers/ (follow the structure/pattern of existing
routers like config, ui-state, window), move each ipc handler into typed tRPC
procedures that accept the same single-object params, remove the ipcMain.handle
calls, and then integrate/register the new storage router in the main tRPC setup
so the desktop process exposes the same functionality via tRPC instead of direct
ipcMain.
| export function registerStorageHandlers() { | ||
| ipcMain.handle("storage:get", async (_event, input: { key: string }) => { | ||
| return store.get(input.key); | ||
| }); | ||
|
|
||
| ipcMain.handle( | ||
| "storage:set", | ||
| async (_event, input: { key: string; value: unknown }) => { | ||
| store.set(input.key, input.value); | ||
| }, | ||
| ); | ||
|
|
||
| ipcMain.handle("storage:delete", async (_event, input: { key: string }) => { | ||
| store.delete(input.key); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent unhandled exceptions.
The handlers directly delegate to store methods without try-catch. If the store operations fail (e.g., disk write error, invalid key format), the errors will propagate as unhandled rejections.
export function registerStorageHandlers() {
ipcMain.handle("storage:get", async (_event, input: { key: string }) => {
- return store.get(input.key);
+ try {
+ return store.get(input.key);
+ } catch (error) {
+ console.error("[storage-ipcs] Failed to get key:", input.key, error);
+ return null;
+ }
});
ipcMain.handle(
"storage:set",
async (_event, input: { key: string; value: unknown }) => {
- store.set(input.key, input.value);
+ try {
+ store.set(input.key, input.value);
+ } catch (error) {
+ console.error("[storage-ipcs] Failed to set key:", input.key, error);
+ }
},
);
ipcMain.handle("storage:delete", async (_event, input: { key: string }) => {
- store.delete(input.key);
+ try {
+ store.delete(input.key);
+ } catch (error) {
+ console.error("[storage-ipcs] Failed to delete key:", input.key, error);
+ }
});
}📝 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.
| export function registerStorageHandlers() { | |
| ipcMain.handle("storage:get", async (_event, input: { key: string }) => { | |
| return store.get(input.key); | |
| }); | |
| ipcMain.handle( | |
| "storage:set", | |
| async (_event, input: { key: string; value: unknown }) => { | |
| store.set(input.key, input.value); | |
| }, | |
| ); | |
| ipcMain.handle("storage:delete", async (_event, input: { key: string }) => { | |
| store.delete(input.key); | |
| }); | |
| } | |
| export function registerStorageHandlers() { | |
| ipcMain.handle("storage:get", async (_event, input: { key: string }) => { | |
| try { | |
| return store.get(input.key); | |
| } catch (error) { | |
| console.error("[storage-ipcs] Failed to get key:", input.key, error); | |
| return null; | |
| } | |
| }); | |
| ipcMain.handle( | |
| "storage:set", | |
| async (_event, input: { key: string; value: unknown }) => { | |
| try { | |
| store.set(input.key, input.value); | |
| } catch (error) { | |
| console.error("[storage-ipcs] Failed to set key:", input.key, error); | |
| } | |
| }, | |
| ); | |
| ipcMain.handle("storage:delete", async (_event, input: { key: string }) => { | |
| try { | |
| store.delete(input.key); | |
| } catch (error) { | |
| console.error("[storage-ipcs] Failed to delete key:", input.key, error); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/storage-ipcs.ts around lines 8 to 23, the IPC
handlers call store methods directly and lack try/catch, so store failures can
become unhandled exceptions; wrap each handler body in a try/catch, await the
store operation, return the expected success value (or undefined for
setters/deletes) when successful, and in the catch log the error (including
operation and key) and rethrow a sanitized Error or return a structured error
object so the renderer gets a predictable failure shape.
| import Store from "electron-store"; | ||
| import { SUPERSET_HOME_DIR } from "./app-environment"; | ||
|
|
||
| /** | ||
| * Electron store instance for persisting application state | ||
| * Stores data in ~/.superset/app-state.json (prod) or ~/.superset-dev/app-state.json (dev) | ||
| */ | ||
| export const store = new Store({ | ||
| cwd: SUPERSET_HOME_DIR, | ||
| name: "app-state", | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking for electron-store in package.json files =="
fd package.json -t f | while read -r file; do
echo "--- $file ---"
# Print dependency entries (may output null if not present)
jq '.dependencies?.["electron-store"], .devDependencies?.["electron-store"]' "$file" 2>/dev/null || echo "Could not parse file"
done
echo ""
echo "== Searching desktop bundler configs for electron-store externals =="
fd -t f -E node_modules -E dist -E build 'vite.config|rollup.config' apps/desktop 2>/dev/null || echo "(no configs found with glob pattern)"
echo ""
echo "== Broader search for vite/rollup config in apps/desktop =="
find apps/desktop -maxdepth 3 -name "*.config.*" -type f 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/desktop/package.json =="
cat apps/desktop/package.json | head -100
echo ""
echo "== apps/desktop/electron.vite.config.ts =="
cat apps/desktop/electron.vite.config.tsRepository: superset-sh/superset
Length of output: 5952
Add electron-store to dependencies and externalize it in the main bundle config
The import of electron-store will fail because the dependency is missing from apps/desktop/package.json and not configured as external in electron.vite.config.ts.
Required fixes:
- Add
electron-storeto thedependenciessection inapps/desktop/package.json(it's currently absent from the ~60 listed packages). - Add
"electron-store"to theexternalarray inelectron.vite.config.tsunder themain.build.rollupOptionssection (line 32–47), since it's a native Electron module that should not be bundled.
The storage manager design itself is sound: a single Store instance scoped to SUPERSET_HOME_DIR with the persistent config name "app-state" is reasonable.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Rollup failed to resolve import 'electron-store'. This is likely due to a missing dependency or misconfigured build; consider adding 'electron-store' to dependencies or externalizing it in build.rollupOptions.external.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/storage-manager.ts lines 1–11, the code imports
"electron-store" but the package is missing from apps/desktop/package.json and
not marked external in the main build config; add "electron-store" to the
dependencies section of apps/desktop/package.json, and add "electron-store" to
the external array in electron.vite.config.ts under main.build.rollupOptions
(around lines 32–47) so the native Electron module is not bundled by
Vite/Rollup.
| electronStore: { | ||
| get: (key: string) => unknown; | ||
| set: (key: string, value: unknown) => void; | ||
| delete: (key: string) => void; | ||
| }; |
There was a problem hiding this comment.
Window interface types don't match async return values.
The electronStore interface declares synchronous return types (unknown, void), but the actual implementation uses ipcRenderer.invoke() which returns Promises. This type mismatch will cause TypeScript to incorrectly type-check consumer code.
electronStore: {
- get: (key: string) => unknown;
- set: (key: string, value: unknown) => void;
- delete: (key: string) => void;
+ get: (key: string) => Promise<unknown>;
+ set: (key: string, value: unknown) => Promise<void>;
+ delete: (key: string) => Promise<void>;
};🤖 Prompt for AI Agents
In apps/desktop/src/preload/index.ts around lines 13–17, the electronStore type
declares synchronous signatures but the implementation uses ipcRenderer.invoke
(which returns Promises); update the interface so get returns a Promise (e.g.
get<T = unknown>(key: string): Promise<T | undefined>) and set/delete return
Promise<void> (or Promise<boolean> if implementation resolves success), adjust
generics/nullable return as appropriate, and update any callers if needed to
await the results.
| // Play the new sound | ||
| setPlayingId(ringtone.id); | ||
|
|
||
| try { | ||
| await window.ipcRenderer.invoke("ringtone:preview", { | ||
| filename: ringtone.filename, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to play ringtone:", error); | ||
| setPlayingId(null); | ||
| } |
There was a problem hiding this comment.
Potential state inconsistency on failed preview.
Setting playingId before the IPC call completes (line 201) means the UI shows "playing" state even if the preview fails. Consider setting the state after successful invocation for more accurate UI feedback.
- // Play the new sound
- setPlayingId(ringtone.id);
-
try {
await window.ipcRenderer.invoke("ringtone:preview", {
filename: ringtone.filename,
});
+ // Only set playing state after successful invocation
+ setPlayingId(ringtone.id);
} catch (error) {
console.error("Failed to play ringtone:", error);
- setPlayingId(null);
+ return; // Don't set up the auto-reset timer on failure
}🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx
around lines 200–210, the component sets playingId before the IPC preview
completes which can leave the UI showing "playing" when the preview fails; move
setPlayingId(ringtone.id) so it runs only after the await
window.ipcRenderer.invoke(...) resolves successfully, and keep
setPlayingId(null) inside the catch to clear the state on failure (optionally
add a local "loading" flag if you need immediate visual feedback while the IPC
call is pending).
AviPeltz
force-pushed
the
quarrelsome-felidae-740317
branch
from
5c7ee1e to
dd0fff7
Compare
AviPeltz
force-pushed
the
quarrelsome-felidae-740317
branch
from
dd0fff7 to
7b40d74
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (1)
151-220: IPC preview/stop flow and timers are solid; consider a couple of robustness tweaksThe overall flow is good:
useEffectcleanup stops any preview and clears the timer on unmount, avoiding setState-after-unmount.handleTogglePlaycorrectly:
- short-circuits for
"none"/missing filename,- enforces a single active preview (stop before play),
- auto-resets
playingIdafterduration + 0.5s.Two small, non-blocking refinements:
Guarding
window.ipcRenderer
If this component is ever rendered in a non-Electron context, direct calls likewindow.ipcRenderer.invoke(...)will throw synchronously before the.catch. Wrapping those calls with a check (e.g.,if (!window.ipcRenderer) return;) would make the component safer in tests.Aligning UI state with preview success
setPlayingId(ringtone.id)is called before theawait window.ipcRenderer.invoke("ringtone:preview", ...). If the preview fails, the UI can briefly show a “playing” state that never actually plays. MovingsetPlayingId(ringtone.id)inside thetryblock after the await (and skipping the auto-reset timer on failure) would keep the UI strictly in sync with successful previews. This has been noted previously and remains an optional polish rather than a blocker.apps/desktop/src/main/lib/storage-manager.ts (1)
1-12:electron-storedependency and types are missing (build is currently broken)TypeScript cannot resolve
"electron-store"here, and the main bundle will also fail to resolve it unless the dependency and bundler config are updated.Concrete follow-ups:
- Add
electron-storetoapps/desktop/package.jsondependencies(and, if needed, its@typespackage).- Ensure the main Electron bundle treats
"electron-store"as external inelectron.vite.config.ts(or equivalent), so it’s not bundled.The storage design itself (single
StoreunderSUPERSET_HOME_DIR, separatesettings.jsonfromapp-state.json) looks solid; this is just a wiring/dependency issue.apps/desktop/src/preload/index.ts (1)
13-17:Window.electronStoreis typed as sync but implemented as async Promises
electronStoreAPIusesipcRenderer.invoke(...), which returnsPromises, but theWindow.electronStoreinterface declares synchronous return types (unknown,void). This will mislead consumers (e.g., cannot safelyawaitwindow.electronStore.getwithout casting) and breaks type safety.Consider updating the interface to reflect the actual async contract and add a generic for better typing:
- electronStore: { - get: (key: string) => unknown; - set: (key: string, value: unknown) => void; - delete: (key: string) => void; - }; + electronStore: { + get<T = unknown>(key: string): Promise<T | undefined>; + set<T = unknown>(key: string, value: T): Promise<void>; + delete(key: string): Promise<void>; + };
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/notification-sound.ts (1)
15-22: Consider centralizing ringtone path + playback helpers to avoid drift with IPC handlers
getRingtonePathandplaySoundFilehere are conceptually the same utilities used by the ringtone IPC handlers (e.g., inringtone-ipcs.ts). Duplicating this logic in two places (dev vs. prod path resolution, platform-specific playback) risks subtle divergence over time.Consider extracting a shared main-process helper module (e.g.,
lib/ringtones/audio-utils.ts) used by both:
playNotificationSound(); and- the IPC preview handlers.
That keeps path computation, supported formats, and playback behavior in one place.
Also applies to: 69-105
apps/desktop/src/preload/index.ts (1)
84-90: Reuse typed IPC wrapper for storage channels and align with IPC guidelinesThe new
electronStoreAPIdirectly callsipcRenderer.invoke("storage:*", ...), even though this file already defines a typedipcRendererAPIbased onIpcChannelName/IpcRequest/IpcResponse_. Using the typed wrapper would give you compiler-checked channel names and payloads and keep all non‑tRPC IPC going through one surface.You can keep behavior the same while tightening types:
-const electronStoreAPI = { - get: (key: string) => ipcRenderer.invoke("storage:get", { key }), - set: (key: string, value: unknown) => - ipcRenderer.invoke("storage:set", { key, value }), - delete: (key: string) => ipcRenderer.invoke("storage:delete", { key }), -}; +const electronStoreAPI = { + get: (key: string) => ipcRendererAPI.invoke("storage:get", { key }), + set: (key: string, value: unknown) => + ipcRendererAPI.invoke("storage:set", { key, value }), + delete: (key: string) => ipcRendererAPI.invoke("storage:delete", { key }), +};Longer‑term, given the desktop guidelines to route Electron IPC through tRPC where possible, you may also want to consider exposing this storage surface via a small tRPC router instead of custom
"storage:*"channels, to keep all IPC consistent. Based on learnings and coding guidelines, this is a recommended follow‑up, not a blocker for this PR.Also applies to: 94-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
apps/desktop/src/resources/sounds/agentisdonewoman.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteafrican.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteafrobeat.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteedm.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/comebacktothecode.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/shabalabadingdong.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/supersetdoowap.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/supersetquick.mp3is excluded by!**/*.mp3bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/notification-sound.ts(2 hunks)apps/desktop/src/main/lib/ringtone-ipcs.ts(1 hunks)apps/desktop/src/main/lib/storage-ipcs.ts(1 hunks)apps/desktop/src/main/lib/storage-manager.ts(1 hunks)apps/desktop/src/preload/index.ts(2 hunks)apps/desktop/src/renderer/lib/electron-storage.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx(2 hunks)apps/desktop/src/renderer/stores/app-state.ts(1 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/renderer/stores/ringtone/index.ts(1 hunks)apps/desktop/src/renderer/stores/ringtone/store.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(2 hunks)apps/desktop/src/shared/ipc-channels/ringtone.ts(1 hunks)apps/desktop/src/shared/ipc-channels/storage.ts(1 hunks)apps/desktop/src/shared/ringtones.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/desktop/src/main/lib/storage-ipcs.ts
- apps/desktop/src/renderer/stores/index.ts
- apps/desktop/src/renderer/stores/ringtone/index.ts
- apps/desktop/src/renderer/stores/ringtone/store.ts
- apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx
- apps/desktop/src/main/lib/ringtone-ipcs.ts
- apps/desktop/src/shared/ipc-channels/storage.ts
- apps/desktop/src/shared/ipc-channels/ringtone.ts
- apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
- apps/desktop/src/shared/ringtones.ts
🧰 Additional context used
📓 Path-based instructions (6)
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/storage-manager.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/shared/ipc-channels/index.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/storage-manager.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/shared/ipc-channels/index.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/storage-manager.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/shared/ipc-channels/index.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env file in src/main/index.ts with override: true
Files:
apps/desktop/src/main/index.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - only in src/main/
Files:
apps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.tsapps/desktop/src/renderer/lib/electron-storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx
**/components/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one folder per component with structure ComponentName/ComponentName.tsx + index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx
🧠 Learnings (6)
📚 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/main/lib/*{workspace,worktree}-*.ts : Use git worktree-based workspace management in the desktop app through workspace-manager.ts and worktree-manager.ts
Applied to files:
apps/desktop/src/main/lib/storage-manager.tsapps/desktop/src/preload/index.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/lib/storage-manager.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/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/shared/ipc-channels/index.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/main/lib/*-ipcs.ts : Use type-safe IPC system with handlers accepting single object parameters, not positional parameters
Applied to files:
apps/desktop/src/main/index.tsapps/desktop/src/shared/ipc-channels/index.ts
📚 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/preload/index.tsapps/desktop/src/renderer/lib/electron-storage.ts
📚 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} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/lib/electron-storage.ts
🧬 Code graph analysis (4)
apps/desktop/src/main/lib/storage-manager.ts (1)
apps/desktop/src/main/lib/app-environment.ts (1)
SUPERSET_HOME_DIR(8-8)
apps/desktop/src/main/index.ts (2)
apps/desktop/src/main/lib/storage-ipcs.ts (1)
registerStorageHandlers(8-23)apps/desktop/src/main/lib/ringtone-ipcs.ts (1)
registerRingtoneHandlers(87-148)
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (2)
apps/desktop/src/renderer/stores/ringtone/store.ts (4)
Ringtone(11-11)useSelectedRingtoneId(61-62)useSetRingtone(63-64)AVAILABLE_RINGTONES(12-12)packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/shared/ipc-channels/index.ts (3)
apps/desktop/src/shared/ipc-channels/ui.ts (1)
UiChannels(8-83)apps/desktop/src/shared/ipc-channels/storage.ts (1)
StorageChannels(5-20)apps/desktop/src/shared/ipc-channels/ringtone.ts (1)
RingtoneChannels(7-31)
🪛 GitHub Actions: CI
apps/desktop/src/main/lib/storage-manager.ts
[error] 1-19: Cannot find module 'electron-store' or its corresponding type declarations. (TS2307).
🔇 Additional comments (7)
apps/desktop/src/main/index.ts (1)
8-9: Early IPC handler registration looks good; just ensure handlers don’t depend on app readinessRegistering
registerStorageHandlers()andregisterRingtoneHandlers()at module load time is fine foripcMain.handlewiring and keeps IPC available before windows are created. Please just confirm that the underlying handlers don’t synchronously accessBrowserWindowor other APIs that requireapp.whenReady(); deferred work should happen only when an IPC call actually arrives.Also applies to: 33-34
apps/desktop/src/shared/ipc-channels/index.ts (1)
11-12: IPC channel typing extension is consistentIncluding
StorageChannelsandRingtoneChannelsinIpcChannelskeeps the channel name union and request/response typings in sync with the new IPC surfaces. This matches the contracts defined instorage.tsandringtone.ts.Also applies to: 36-38
apps/desktop/src/main/lib/notification-sound.ts (1)
24-64: Ringtone selection decoding and fallbacks look robust; just confirm persisted shape/keyThe
getSelectedRingtoneFilename()logic handles:
- Missing/invalid storage by falling back to
DEFAULT_RINGTONE_ID.- The zustand
persistshape ({ state: { selectedRingtoneId }, ... }), both when stored as a JSON string and when auto-parsed.- The
"none"sentinel by returning""soplayNotificationSoundbecomes a no-op.This should behave correctly as long as:
- The persist name used in the renderer store is exactly
"ringtone-storage".- The stored value really matches
PersistedRingtoneState(i.e., it has astate.selectedRingtoneIdfield).If those ever change, reads here will quietly fall back to the default ringtone, so tests around “non-default ringtone plays on notification” would be useful to catch drift.
apps/desktop/src/renderer/stores/app-state.ts (1)
5-11: New settings sections wiring is straightforwardAdding
"presets"and"ringtones"toSettingsSectionkeeps the store aligned with the new Settings panels and sidebar entries, without impacting the default section ("project").apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.ts (1)
1-1: Component barrel export matches the component-folder conventionRe-exporting
RingtonesSettingsviaindex.tsaligns with the “one folder per component + barrel export” pattern and keeps imports clean.apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (2)
23-72: RingtoneCard structure and HTML semantics resolve the nested-button issueUsing:
- A plain
<button>for the Silent card, and- A
<div role="button" tabIndex={0}>wrapper with a nested play/stop<button>for regular cards,avoids invalid nested buttons while keeping keyboard accessibility (Enter/Space) and click behavior. The visual treatments for the Silent card (dashed border + bell-slash icon) and the duration badge on regular cards are clear UX improvements.
Also applies to: 74-147
229-265: Settings panel UX for ringtones is clear and consistentThe layout (title + description, grid of cards, and a brief usage tip) integrates well with the existing Settings view patterns. Mapping
AVAILABLE_RINGTONESintoRingtoneCardinstances withselectedRingtoneId/playingIddrives the UI deterministically from state.
| /** | ||
| * Custom Zustand storage adapter that uses electron-store for persistence via IPC | ||
| * Stores state in ~/.superset/app-state.json | ||
| */ | ||
| const electronStorageAdapter = { | ||
| getItem: async (name: string): Promise<string | null> => { | ||
| const value = await window.electronStore.get(name); | ||
| return value as string | null; | ||
| }, | ||
| setItem: async (name: string, value: string): Promise<void> => { | ||
| await window.electronStore.set(name, value); | ||
| }, | ||
| removeItem: async (name: string): Promise<void> => { | ||
| await window.electronStore.delete(name); | ||
| }, | ||
| }; | ||
|
|
||
| export const electronStorage = createJSONStorage(() => electronStorageAdapter); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the main process electron-store configuration
find apps/desktop -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "new Store" 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 108
🏁 Script executed:
# Search for Store initialization and the filename/name configuration
rg "new Store\(" apps/desktop/src/main -A 5 -B 2Repository: superset-sh/superset
Length of output: 489
🏁 Script executed:
# Check for preload script and window.electronStore definition
find apps/desktop -type f \( -name "*preload*" -o -name "*bridge*" \)Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for window.electronStore usage and type definitions
rg "electronStore\s*=" apps/desktop/src -B 2 -A 5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if tRPC is used for IPC communication in this codebase
rg "tRPC|trpc" apps/desktop/src --type ts --type tsx | head -20Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for preload script setup
find apps/desktop -name "*.ts" -o -name "*.js" | xargs grep -l "contextBridge\|preload" 2>/dev/nullRepository: superset-sh/superset
Length of output: 223
🏁 Script executed:
# Look for window type definitions or electronStore
rg "window\.electronStore" apps/desktop/src --type ts -B 3 -A 3Repository: superset-sh/superset
Length of output: 1173
🏁 Script executed:
# Check for IPC handlers related to store
rg "ipcMain\.(handle|on).*store\|electron\.store" apps/desktop/src/main -B 2 -A 5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for tRPC router definitions in main process
find apps/desktop/src/main -name "*.ts" | xargs grep -l "router\|createTRPCRouter" 2>/dev/nullRepository: superset-sh/superset
Length of output: 101
🏁 Script executed:
# Read the preload script to understand window.electronStore setup
cat apps/desktop/src/preload/index.tsRepository: superset-sh/superset
Length of output: 3354
🏁 Script executed:
# Read type definitions
cat apps/desktop/index.d.tsRepository: superset-sh/superset
Length of output: 836
🏁 Script executed:
# Search for how the store is exposed to renderer process
rg "electronStore\|store" apps/desktop/src/preload -A 5 -B 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's a tRPC route for storage operations
find apps/desktop/src/main -name "*.ts" | xargs grep -l "storage" | xargs grep -l "router\|procedure"Repository: superset-sh/superset
Length of output: 48
🏁 Script executed:
# Look for tRPC router structure in main process
rg "createTRPCRouter\|\.query\|\.mutation" apps/desktop/src/main --type ts -B 2 -A 8 | head -50Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check src/lib/trpc to understand the tRPC setup
find apps/desktop/src -path "*lib/trpc*" -type fRepository: superset-sh/superset
Length of output: 2320
🏁 Script executed:
# Check the existing trpc-storage.ts file
cat apps/desktop/src/renderer/lib/trpc-storage.tsRepository: superset-sh/superset
Length of output: 2119
🏁 Script executed:
# Check the settings router
cat apps/desktop/src/lib/trpc/routers/settings/index.tsRepository: superset-sh/superset
Length of output: 2103
🏁 Script executed:
# Check if there are any storage-related procedures in routers
rg "storage|store" apps/desktop/src/lib/trpc/routers -B 2 -A 5Repository: superset-sh/superset
Length of output: 46
Update docstring filename and refactor to use tRPC for IPC instead of direct ipcRenderer
The async adapter shape is appropriate for Zustand, but two issues need addressing:
-
The docstring mentions
~/.superset/app-state.json, but the main-processStoreis configured withname: "settings"(i.e.,settings.json). Update the comment to match. -
This code violates the coding guideline requiring tRPC for all Electron IPC in
apps/desktop/**. Replace the directipcRenderer.invoke()calls with a tRPC-based storage adapter, following the pattern inapps/desktop/src/renderer/lib/trpc-storage.ts. Add storage procedures to the appropriate tRPC router so the adapter can use typed tRPC client queries/mutations instead of raw IPC channels.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (1)
151-219: Preview control and cleanup logic are robust; minor optional tweak for failed previewsThe play/stop handler correctly clears any pending timer, serializes
ringtone:stop/ringtone:previewcalls, and auto-resetsplayingIdbased on the ringtone’s duration, with unmount cleanup stopping any in-flight preview. The only minor edge case is thatplayingIdis set beforeringtone:previewresolves, so the UI can briefly show “playing” even if the IPC call rejects before you clear it in thecatch; if you care about that, you could movesetPlayingId(ringtone.id)into the successfultryblock and skip scheduling the auto-reset timer when previewing fails.apps/desktop/src/main/lib/ringtone-ipcs.ts (3)
46-54: MakestopCurrentSoundcross‑platform safe (unconditionalSIGKILL).
stopCurrentSoundalways callscurrentSession.process.kill("SIGKILL")(Line 50). This can behave inconsistently on Windows and was already flagged earlier. Prefer a platform-conditional approach so Windows uses the default/SIGTERMbehavior while Unix keeps the hard kill:function stopCurrentSound(): void { if (currentSession) { if (currentSession.process) { - // Use SIGKILL for immediate termination (afplay doesn't always respond to SIGTERM) - currentSession.process.kill("SIGKILL"); + // Use platform-appropriate termination + if (process.platform === "win32") { + // Default signal is equivalent to a terminate request on Windows + currentSession.process.kill(); + } else { + // Unix: keep SIGKILL for immediate termination + currentSession.process.kill("SIGKILL"); + } } currentSession = null; } }
80-89: Escape or parameterizesoundPathin the PowerShell command.On Windows,
soundPathis interpolated directly into a PowerShell script string (Line 83). If the path ever contains single quotes or certain characters, it can break the script or enable command injection in principle. At minimum, escape single quotes for PowerShell single‑quoted strings:} else if (process.platform === "win32") { - currentSession.process = execFile( - "powershell", - ["-c", `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`], + // Escape single quotes for PowerShell single-quoted string literal + const escapedPath = soundPath.replace(/'/g, "''"); + currentSession.process = execFile( + "powershell", + ["-c", `(New-Object Media.SoundPlayer '${escapedPath}').PlaySync()`], () => { if (currentSession?.id === sessionId) { currentSession = null; } }, );This keeps the current invocation style but avoids malformed scripts when paths contain quotes.
115-176: Align ringtone IPC with the tRPC-based IPC pattern.
registerRingtoneHandlersstill uses rawipcMain.handlefor"ringtone:preview","ringtone:stop", and"ringtone:list". Per the desktop coding guidelines and prior review, Electron IPC in this app is expected to go through the tRPC router insrc/lib/trpc, with handlers exposed as procedures rather than ad‑hoc channels.Consider:
- Extracting the core operations into plain functions (e.g.,
previewRingtone({ filename }),stopRingtone(),listRingtones()).- Creating a
ringtonetRPC router (e.g.,apps/desktop/src/lib/trpc/routers/ringtone.ts) that calls these functions and returns the same{ success, data?, error? }shape.- Wiring that router into the main tRPC router aggregator so the renderer uses tRPC instead of
ipcRenderer.invoke("ringtone:*").- Removing the direct
ipcMain.handleregistrations from this file once the router is in place.This keeps the business logic here but moves IPC wiring into the standardized tRPC layer.
As per coding guidelines, Electron IPC should use tRPC.
🧹 Nitpick comments (4)
apps/desktop/src/main/lib/storage-manager.ts (1)
1-12: Electron Store setup is straightforward and isolated to a dedicated settings fileUsing
SUPERSET_HOME_DIRwith a separate"settings"store cleanly avoids collisions withapp-state.json; I don't see functional issues here. If you later want stronger guarantees, you could parameterizeStorewith a schema interface for the known keys rather than relying on the default loose typing.apps/desktop/src/shared/ringtones.ts (1)
1-129: Shared ringtone catalog API is well-structured; consider tightening ID typing laterThe
RINGTONESarray,DEFAULT_RINGTONE_ID, and thegetRingtoneById/getRingtoneFilenamehelpers give both main and renderer a single, consistent source of truth, and the Silent entry with an empty filename lines up with the silent handling elsewhere. As a future enhancement, you could derive a string-literal union of valid IDs fromRINGTONES(or a manual union type) and use that inRingtoneData["id"]/call sites for even stronger type safety.apps/desktop/src/main/lib/ringtone-ipcs.ts (2)
12-17: Session tracking design looks good; minor optional improvement.The
currentSession+sessionIdpattern cleanly prevents stale callbacks (especially in the Linux fallback) from mutating state afterstopCurrentSound()is called. The monotonically increasingnextSessionIdis fine in practice, but if you want to be extra defensive you could wrap it when it approachesNumber.MAX_SAFE_INTEGERor use aSymbol/UUID instead of a numeric counter.Also applies to: 70-72
118-127: Consider stopping current sound even when previewing “none”/empty filename.In
"ringtone:preview", the early return on empty filename (Lines 121–123) skipsplaySoundFile, so any currently playing preview keeps running if this path is ever used for a “silent”/“none” selection.If this handler can be invoked when a user selects the Silent option, you probably want it to stop any existing preview as well:
try { // Handle "none" case - no sound if (!input.filename || input.filename === "") { - return { success: true }; + // Ensure any existing preview is stopped when choosing silence + stopCurrentSound(); + return { success: true }; }This is behaviorally safe even if the “none” path is rare, and makes the semantics clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
apps/desktop/src/resources/sounds/agentisdonewoman.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteafrican.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteafrobeat.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/codecompleteedm.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/comebacktothecode.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/shabalabadingdong.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/supersetdoowap.mp3is excluded by!**/*.mp3apps/desktop/src/resources/sounds/supersetquick.mp3is excluded by!**/*.mp3bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/notification-sound.ts(2 hunks)apps/desktop/src/main/lib/ringtone-ipcs.ts(1 hunks)apps/desktop/src/main/lib/storage-ipcs.ts(1 hunks)apps/desktop/src/main/lib/storage-manager.ts(1 hunks)apps/desktop/src/preload/index.ts(2 hunks)apps/desktop/src/renderer/lib/electron-storage.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx(2 hunks)apps/desktop/src/renderer/stores/app-state.ts(1 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/renderer/stores/ringtone/index.ts(1 hunks)apps/desktop/src/renderer/stores/ringtone/store.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(2 hunks)apps/desktop/src/shared/ipc-channels/ringtone.ts(1 hunks)apps/desktop/src/shared/ipc-channels/storage.ts(1 hunks)apps/desktop/src/shared/ringtones.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
- apps/desktop/src/main/index.ts
- apps/desktop/src/shared/ipc-channels/storage.ts
- apps/desktop/src/renderer/stores/ringtone/store.ts
- apps/desktop/src/main/lib/storage-ipcs.ts
- apps/desktop/src/renderer/stores/ringtone/index.ts
- apps/desktop/src/preload/index.ts
- apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/index.ts
- apps/desktop/src/renderer/lib/electron-storage.ts
- apps/desktop/src/renderer/stores/app-state.ts
🧰 Additional context used
📓 Path-based instructions (6)
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/storage-manager.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/shared/ringtones.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/shared/ipc-channels/ringtone.tsapps/desktop/src/renderer/stores/index.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/storage-manager.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/shared/ringtones.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/shared/ipc-channels/ringtone.tsapps/desktop/src/renderer/stores/index.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/storage-manager.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/shared/ringtones.tsapps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/shared/ipc-channels/ringtone.tsapps/desktop/src/renderer/stores/index.ts
**/components/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one folder per component with structure ComponentName/ComponentName.tsx + index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - only in src/main/
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/stores/index.ts
apps/desktop/src/main/lib/*-ipcs.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system with handlers accepting single object parameters, not positional parameters
Files:
apps/desktop/src/main/lib/ringtone-ipcs.ts
🧠 Learnings (7)
📚 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/main/lib/*{workspace,worktree}-*.ts : Use git worktree-based workspace management in the desktop app through workspace-manager.ts and worktree-manager.ts
Applied to files:
apps/desktop/src/main/lib/storage-manager.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/lib/storage-manager.tsapps/desktop/src/renderer/stores/index.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/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/shared/ipc-channels/ringtone.ts
📚 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/lib/ringtone-ipcs.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/main/lib/*-ipcs.ts : Use type-safe IPC system with handlers accepting single object parameters, not positional parameters
Applied to files:
apps/desktop/src/main/lib/ringtone-ipcs.tsapps/desktop/src/shared/ipc-channels/index.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/main/lib/terminal-*.ts : Use node-pty for terminal session management in the desktop app
Applied to files:
apps/desktop/src/main/lib/ringtone-ipcs.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/renderer/stores/index.ts
🧬 Code graph analysis (4)
apps/desktop/src/main/lib/storage-manager.ts (1)
apps/desktop/src/main/lib/app-environment.ts (1)
SUPERSET_HOME_DIR(8-8)
apps/desktop/src/shared/ringtones.ts (1)
apps/desktop/src/renderer/stores/ringtone/store.ts (1)
DEFAULT_RINGTONE_ID(13-13)
apps/desktop/src/main/lib/notification-sound.ts (2)
apps/desktop/src/shared/ringtones.ts (2)
getRingtoneFilename(126-129)DEFAULT_RINGTONE_ID(113-113)apps/desktop/src/main/lib/storage-manager.ts (1)
store(9-12)
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (2)
apps/desktop/src/renderer/stores/ringtone/store.ts (4)
Ringtone(11-11)useSelectedRingtoneId(61-62)useSetRingtone(63-64)AVAILABLE_RINGTONES(12-12)packages/ui/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (8)
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx (1)
3-39: Ringtones sidebar section wiring looks consistentThe new
"ringtones"entry (withHiOutlineBell) matches theSettingsSectiontype and integrates cleanly intoGENERAL_SECTIONS, so navigation to the new settings pane should work without additional plumbing here.apps/desktop/src/renderer/stores/index.ts (1)
7-11: Barrel export for ringtone store keeps store imports centralizedRe-exporting from
"./ringtone"here is aligned with how other stores are exposed and keeps consumer import paths consistent.apps/desktop/src/shared/ipc-channels/ringtone.ts (1)
1-31: Ringtone IPC channel typings are clear and consistentThe three channels (
ringtone:preview,ringtone:stop,ringtone:list) follow the existing IPC pattern with single-object requests andIpcResponse-wrapped responses, which should make wiring handlers and renderer calls straightforward.apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (1)
11-149: Card UI and a11y handling for ringtones (including Silent) look solidThe split between a real
<button>for the Silent card and adiv role="button"wrapper for playable cards avoids nested-button HTML while still providing keyboard interaction viaonKeyDown. Duration badges and play/stop controls are wired in a clean, per-card way withisSelected/isPlayingprops, which keeps the markup predictable.apps/desktop/src/shared/ipc-channels/index.ts (1)
11-38: ExtendingIpcChannelswith storage and ringtone channels keeps IPC typing centralizedPulling
StorageChannelsandRingtoneChannelsinto theIpcChannelsunion matches how other domains are modeled, soIpcChannelNameandIpcRequest/IpcResponse_automatically cover the new functionality without extra plumbing.apps/desktop/src/main/lib/notification-sound.ts (1)
24-106: Storage key alignment is correct; persist configuration properly matches notification-sound.ts lookupThe zustand persist middleware in
apps/desktop/src/renderer/stores/ringtone/store.tsis configured withname: "ringtone-storage"and useselectronStorage, which aligns perfectly with thestore.get("ringtone-storage")call ingetSelectedRingtoneFilename. ThegetSelectedRingtoneFilenamefunction correctly handles both string and object values from the store, treats"none"as intentional silence, and gracefully falls back to the default ringtone when the stored ID is invalid or stale.apps/desktop/src/main/lib/ringtone-ipcs.ts (2)
91-108: Linux fallback race appears resolved.The Linux branch now ties callbacks to
sessionId(Lines 92–107). ThecurrentSession?.id !== sessionIdguard preventsaplayfrom starting afterstopCurrentSound()has invalidated or replaced the session, addressing the earlier race where the fallback process could be orphaned from session tracking.
151-175: Ringtone listing and notification playback behavior look consistent.
"ringtone:list"correctly filters known audio extensions from the resolved sounds directory and guards withexistsSync, whileplayNotificationRingtonemirrors the “empty filename → no sound” behavior using the same path resolution and playback helper. This keeps the notification path and preview path aligned on file resolution and platform-specific playback.Also applies to: 182-189
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/settings/index.ts(2 hunks)apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/db/schemas.ts(1 hunks)apps/desktop/src/main/lib/notification-sound.ts(2 hunks)apps/desktop/src/renderer/lib/trpc-storage.ts(1 hunks)apps/desktop/src/renderer/stores/ringtone/store.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/shared/ipc-channels/index.tsapps/desktop/src/renderer/lib/trpc-storage.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/db/schemas.tsapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/main/lib/notification-sound.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/shared/ipc-channels/index.tsapps/desktop/src/renderer/lib/trpc-storage.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/db/schemas.tsapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/main/lib/notification-sound.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/renderer/lib/trpc-storage.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/db/schemas.tsapps/desktop/src/renderer/stores/ringtone/store.tsapps/desktop/src/main/lib/notification-sound.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - only in src/main/
Files:
apps/desktop/src/renderer/lib/trpc-storage.tsapps/desktop/src/renderer/stores/ringtone/store.ts
apps/desktop/src/lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules like node:fs, node:path, node:os in src/lib/electron-router-dom.ts or similar shared code
Files:
apps/desktop/src/lib/trpc/routers/settings/index.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env file in src/main/index.ts with override: true
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (3)
📚 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/shared/ipc-channels/index.tsapps/desktop/src/main/index.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/main/lib/*-ipcs.ts : Use type-safe IPC system with handlers accepting single object parameters, not positional parameters
Applied to files:
apps/desktop/src/shared/ipc-channels/index.ts
📚 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/renderer/lib/trpc-storage.ts
🧬 Code graph analysis (5)
apps/desktop/src/shared/ipc-channels/index.ts (2)
apps/desktop/src/shared/ipc-channels/ui.ts (1)
UiChannels(8-83)apps/desktop/src/shared/ipc-channels/ringtone.ts (1)
RingtoneChannels(7-31)
apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
apps/desktop/src/shared/ringtones.ts (1)
RINGTONES(21-111)
apps/desktop/src/main/index.ts (1)
apps/desktop/src/main/lib/ringtone-ipcs.ts (1)
registerRingtoneHandlers(115-176)
apps/desktop/src/renderer/stores/ringtone/store.ts (2)
apps/desktop/src/shared/ringtones.ts (2)
RingtoneData(6-15)RINGTONES(21-111)apps/desktop/src/renderer/lib/trpc-storage.ts (1)
trpcRingtoneStorage(68-82)
apps/desktop/src/main/lib/notification-sound.ts (1)
apps/desktop/src/shared/ringtones.ts (2)
getRingtoneFilename(126-129)DEFAULT_RINGTONE_ID(113-113)
⏰ 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 (14)
apps/desktop/src/main/lib/db/schemas.ts (1)
110-110: LGTM!The optional
selectedRingtoneIdfield is properly typed and positioned. Validation is handled at the tRPC layer, which is the appropriate boundary for this type of business logic.apps/desktop/src/lib/trpc/routers/settings/index.ts (3)
3-8: LGTM!Using the shared
RINGTONEScatalog as the single source of truth for validation is excellent design. This prevents drift between the available ringtones and what the settings API will accept.
86-88: LGTM!The fallback to
DEFAULT_RINGTONE_IDensures the query always returns a valid ringtone ID, which aligns with the PR objectives for handling stale or missing IDs gracefully.
90-103: LGTM!The validation logic is clear and explicit. Throwing an error for invalid ringtone IDs prevents corruption of the settings data. The synchronous validation before the async update is the correct order of operations.
apps/desktop/src/renderer/lib/trpc-storage.ts (1)
64-82: LGTM! Type safety improvement noted.The ringtone storage adapter correctly follows the established pattern. The specific type assertion
as { selectedRingtoneId: string }on Line 76 is actually more type-safe than theas anypattern used in the existing adapters (lines 49, 60), which is a positive refinement.apps/desktop/src/main/lib/notification-sound.ts (4)
5-22: LGTM!The path resolution logic correctly handles both development and production environments. Using
app.isPackagedis the standard approach for Electron environment detection.
28-46: LGTM!The fallback logic is well-designed with multiple defensive layers:
- Falls back to
DEFAULT_RINGTONE_IDifselectedRingtoneIdis undefined- Explicitly handles
"none"for silent notifications- Falls back to default filename if the ID is stale/invalid
- Wraps in try-catch for database access errors
This comprehensively addresses the PR objective for handling stale or unknown ringtone IDs.
51-72: LGTM!The platform-specific playback logic is well-structured:
- Pre-flight existence check prevents unhelpful errors
- macOS
afplayis the standard approach- Windows PowerShell
Media.SoundPlayeris appropriate- Linux fallback chain from
paplaytoaplayhandles both PulseAudio and ALSA systems
78-88: LGTM!The refactored
playNotificationSound()is clean and maintainable. Delegating to well-named helpers (getSelectedRingtoneFilename,getRingtonePath,playSoundFile) makes the flow easy to follow and each function easy to test independently.apps/desktop/src/renderer/stores/ringtone/store.ts (5)
1-13: LGTM!The imports are well-organized and the re-exports provide a convenient public API for consumers. The type alias
Ringtone = RingtoneDataimproves ergonomics without introducing duplication.
15-24: LGTM!The interface is clean and well-documented. The explicit guarantee that
getSelectedRingtone()always returns a valid ringtone (never undefined) prevents defensive coding in consumers.
26-40: LGTM!The helper functions are appropriately extracted:
isValidRingtoneId()provides clear validation semanticsgetDefaultRingtone()throwing on missing default is correct—this would indicate a programming error, not a runtime condition
42-89: LGTM! Previous concern addressed.The store implementation is solid with defense-in-depth validation:
setRingtone()(lines 48-55): Validates before setting, preventing invalid IDs from entering stategetSelectedRingtone()(lines 57-68): Falls back to default if current ID is invalid, protecting consumersonRehydrateStorage(lines 76-84): Validates and resets stale IDs on rehydrationThis comprehensively addresses the previous review comment about handling stale persisted ringtone IDs.
91-103: LGTM!The convenience hooks provide clean APIs for different access patterns:
useSelectedRingtoneId(): For components that only need the IDuseSetRingtone(): For components that only set the ringtoneuseSelectedRingtone(): For components that need the full ringtone dataThe
useSelectedRingtone()hook intentionally returns a fallback without side effects (unlikegetSelectedRingtone()which updates state), which is appropriate for a React hook.
| import { initAppState } from "./lib/app-state"; | ||
| import { setupAutoUpdater } from "./lib/auto-updater"; | ||
| import { initDb } from "./lib/db"; | ||
| import { registerRingtoneHandlers } from "./lib/ringtone-ipcs"; |
There was a problem hiding this comment.
Use tRPC for Electron IPC communication as required by coding guidelines.
As per coding guidelines, all Electron interprocess communication must use tRPC as defined in src/lib/trpc. The registerRingtoneHandlers function uses traditional ipcMain.handle() instead of tRPC, violating this requirement.
Based on coding guidelines for apps/desktop/**/*.{ts,tsx,js,jsx}: "For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc"
| registerRingtoneHandlers(); | ||
|
|
There was a problem hiding this comment.
Refactor to use tRPC instead of traditional IPC handlers.
The registration of ringtone IPC handlers here uses traditional ipcMain.handle() instead of tRPC, which violates the project's coding guidelines. Consider refactoring the ringtone communication layer to use the tRPC infrastructure defined in src/lib/trpc.
Based on coding guidelines for apps/desktop/**/*.{ts,tsx,js,jsx}: "For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc"
🤖 Prompt for AI Agents
In apps/desktop/src/main/index.ts around lines 32-33, the call to
registerRingtoneHandlers() registers traditional ipcMain handlers; replace this
with the project's tRPC-based IPC by moving the ringtone handler logic into a
new (or existing) ringtone router under apps/desktop/src/lib/trpc, expose the
same methods via that router, and initialize/register that router with the
Electron tRPC server bootstrap (the create/initialize function used for other
routers) from main/index.ts instead of calling registerRingtoneHandlers. Remove
direct ipcMain.handle usages, import and mount the ringtone router into the trpc
server init flow, and update any consumer calls to call the tRPC client methods;
ensure context types and serialization match existing trpc infra.
1 participant
Summary
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.