fix (desktop): build issue with ringtones/sounds directory#321
fix (desktop): build issue with ringtones/sounds directory#321
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughCentralizes sound resource handling and build packaging for the desktop Electron app, adds a Vite plugin to copy sound assets, restructures router registration to branch on NODE_ENV, and updates ringtone/notification code to use the new centralized sound-paths utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)apps/desktop/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-12-08T23:32:32.232ZApplied to files:
📚 Learning: 2025-11-24T21:33:13.267ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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: 0
🧹 Nitpick comments (3)
apps/desktop/electron.vite.config.ts (1)
34-51: Consider adding error handling for file operations.The
cpSync,rmSync, andmkdirSyncoperations can throw errors (e.g., permission issues, disk full). Consider wrapping in try-catch to prevent build failures from propagating cryptically.function copyResourcesPlugin(): Plugin { return { name: "copy-resources", writeBundle() { const srcDir = resolve(resources, "sounds"); const destDir = resolve(devPath, "resources/sounds"); if (existsSync(srcDir)) { + try { // Clean destination to avoid stale files if (existsSync(destDir)) { rmSync(destDir, { recursive: true }); } mkdirSync(destDir, { recursive: true }); cpSync(srcDir, destDir, { recursive: true }); + } catch (error) { + console.error("[copy-resources] Failed to copy sounds:", error); + } } }, }; }apps/desktop/src/main/lib/notification-sound.ts (1)
45-49: Consider escaping the sound path for PowerShell command injection safety.The path is interpolated directly into a PowerShell command. While the filename is derived from controlled sources (
getRingtoneFilename), a sound file with a single quote in its name could break the command or enable injection.} else if (process.platform === "win32") { execFile("powershell", [ "-c", - `(New-Object Media.SoundPlayer '${soundPath}').PlaySync()`, + `(New-Object Media.SoundPlayer '${soundPath.replace(/'/g, "''")}').PlaySync()`, ]);apps/desktop/src/lib/electron-router-dom.ts (1)
36-44: Remove unnecessary fallback sinceidis required.Line 38 uses
props.id || "main"as a fallback, butidis a required property of typeWindowIdin the function signature. This fallback is dead code.} else { // Preview or Production: load from file with hash routing - const windowId = props.id || "main"; + const windowId = props.id; let url = `/${windowId}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/electron-builder.ts(1 hunks)apps/desktop/electron.vite.config.ts(2 hunks)apps/desktop/src/lib/electron-router-dom.ts(1 hunks)apps/desktop/src/lib/trpc/routers/ringtone/index.ts(4 hunks)apps/desktop/src/main/lib/notification-sound.ts(2 hunks)apps/desktop/src/main/lib/sound-paths.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/sound-paths.tsapps/desktop/electron-builder.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/lib/trpc/routers/ringtone/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/sound-paths.tsapps/desktop/electron-builder.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/lib/trpc/routers/ringtone/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/sound-paths.tsapps/desktop/electron-builder.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/lib/trpc/routers/ringtone/index.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/electron-router-dom.tsapps/desktop/src/lib/trpc/routers/ringtone/index.ts
🧠 Learnings (7)
📓 Common learnings
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
📚 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/sound-paths.tsapps/desktop/electron.vite.config.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} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/main/lib/sound-paths.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/sound-paths.tsapps/desktop/electron-builder.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/lib/trpc/routers/ringtone/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/main/lib/sound-paths.tsapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/lib/trpc/routers/ringtone/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/renderer/**/*.{ts,tsx} : Never import Node.js modules in renderer process or shared code - only in src/main/
Applied to files:
apps/desktop/src/main/lib/sound-paths.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/sound-paths.ts
🧬 Code graph analysis (2)
apps/desktop/src/lib/electron-router-dom.ts (1)
apps/desktop/src/shared/constants.ts (1)
PORTS(12-17)
apps/desktop/src/lib/trpc/routers/ringtone/index.ts (1)
apps/desktop/src/main/lib/sound-paths.ts (2)
getSoundPath(15-18)getSoundsDirectory(26-58)
⏰ 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 (8)
apps/desktop/src/lib/trpc/routers/ringtone/index.ts (2)
107-109: LGTM!The refactor to use centralized
getSoundPathhelper is clean and maintains the same behavior while consolidating path resolution logic.
5-8: No actionable changes needed. The tsconfig.json does not define an alias for themain/libpath (only*→src/*and~/*→./*are configured). The relative import../../../../main/lib/sound-pathsis the correct approach given the current alias configuration.Likely an incorrect or invalid review comment.
apps/desktop/src/main/lib/notification-sound.ts (1)
8-8: LGTM!The import and integration with the centralized
getSoundPathutility is clean and appropriate for main process code.apps/desktop/src/main/lib/sound-paths.ts (2)
1-18: LGTM!Clean implementation of centralized sound path utilities. The
getSoundPathfunction properly delegates togetSoundsDirectoryfor environment-aware path resolution.
26-57: Well-designed environment detection with graceful fallbacks.The three-tier path resolution (production → development → preview) with existence checks and warning logs provides good debugging information when paths are misconfigured. The production path correctly aligns with the
asarUnpackconfiguration inelectron-builder.ts.apps/desktop/src/lib/electron-router-dom.ts (2)
5-10: LGTM!The router configuration is appropriate. The custom
registerRouteimplementation correctly bypasses the dev server port in preview/production modes by loading files directly, which mitigates any port configuration concerns.
1-3: This file does not import Node.js modules, but Router is used in the renderer process.The file correctly avoids importing Node.js modules like
fs,path, oros. However,electron-router-dom.tsis imported at runtime inapps/desktop/src/renderer/routes.tsx(line 1:import { Router }), and the module evaluatesprocess.env.NODE_ENVat load time. TheRouterobject should be exported without relying on environment variables that may not be available or reliable in the renderer process context. Move any NODE_ENV checks to the main process where the router is actually registered (apps/desktop/src/lib/electron-app/factories/windows/create.ts).Likely an incorrect or invalid review comment.
apps/desktop/electron-builder.ts (1)
35-39: No action needed. The asarUnpack path correctly aligns with the files configuration. Thepkg.resourcesfield resolves to"src/resources", which is copied to the"resources"directory in the output. Thesoundsdirectory exists atapps/desktop/src/resources/soundsand will be included in the bundled resources, making the"resources/sounds/**/*"asarUnpack pattern correct.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.