feat: Unified Downloader Implementation#4348
feat: Unified Downloader Implementation#4348digitalnomad91 wants to merge 12 commits intopear-devs:masterfrom
Conversation
…oader-plugins ci: add concurrency settings to prevent workflow cancellation
… prefix - Fix incorrect console output when file already exists: replace broken 'find newest mp3' fallback with pre-resolved expected filename from yt-dlp - Detect yt-dlp 'already downloaded' output and show accurate log messages - Namespace all ytdlp IPC channels (download-song-ytdlp, downloader-ytdlp-feedback, downloader-ytdlp-error-toast, download-playlist-request-ytdlp) to prevent conflicts when both downloader and downloader-ytdlp plugins are enabled - Change ytdlp button text to 'Download (ytdlp)' and use distinct element ID so both download buttons work independently - Fix 'NA - ' filename prefix by using yt-dlp conditional template syntax that only includes artist when metadata is available
fix(downloader-ytdlp): fix file-exists logging, IPC conflicts, and NA prefix
- Update downloader plugin with unified download logic - Add menu integration for downloader - Update UI components for downloader interface - Add i18n strings for downloader features
There was a problem hiding this comment.
Pull request overview
This PR adds a second, yt-dlp–based downloader plugin alongside the existing downloader, introduces a unified “Downloader” parent menu to group both implementations, and adds an adblocker plugin. It also expands i18n resources to cover the new downloader plugin and settings.
Changes:
- Add a new
downloader-ytdlpplugin (main process + renderer UI + menu + styles) with toast-based feedback/errors and yt-dlp executable path configuration. - Update the existing downloader plugin to use non-blocking toast notifications from the backend instead of modal dialogs.
- Introduce a unified “Downloader” menu entry that groups
downloaderanddownloader-ytdlp, and add translations for the new plugin across many locales.
Reviewed changes
Copilot reviewed 64 out of 65 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/downloader/renderer.tsx | Adds renderer-side toast handling for backend notifications. |
| src/plugins/downloader/main/index.ts | Switches backend error + playlist-start UX from dialogs to renderer toasts. |
| src/plugins/downloader-ytdlp/index.ts | New plugin entrypoint/config for yt-dlp downloader variant. |
| src/plugins/downloader-ytdlp/main/index.ts | New yt-dlp backend implementation (download song/playlist, notifications, caching). |
| src/plugins/downloader-ytdlp/main/utils.ts | Utilities for yt-dlp plugin (folder selection, feedback, badge, image crop). |
| src/plugins/downloader-ytdlp/renderer.tsx | New renderer integration (menu button injection + toast UI) for yt-dlp plugin. |
| src/plugins/downloader-ytdlp/menu.ts | New menu for yt-dlp plugin settings (folder, yt-dlp path, presets, skip-existing). |
| src/plugins/downloader-ytdlp/templates/download.tsx | New/duplicated download button template for yt-dlp plugin UI injection. |
| src/plugins/downloader-ytdlp/style.css | Styles for yt-dlp downloader menu/button elements. |
| src/plugins/downloader-ytdlp/types.ts | Presets + large YouTube format table for the yt-dlp plugin. |
| src/plugins/adblocker/index.ts | Adds an adblocker plugin with multiple modes and preload injection path. |
| src/plugins/adblocker/types/index.ts | Defines adblocker mode constants used by the plugin UI/config. |
| src/plugins/adblocker/blocker.ts | Implements Ghostery ElectronBlocker engine loading/unloading and caching. |
| src/plugins/adblocker/adSpeedup.ts | Adds “ad speedup” mode (mutation observer to speed/mute/skip ads). |
| src/plugins/adblocker/injectors/inject.js | Adds in-player JSON pruning/injection logic for ad removal. |
| src/plugins/adblocker/injectors/inject.d.ts | Type declarations for the JS injector module. |
| src/plugins/adblocker/injectors/inject-cliqz-preload.ts | Preload helper importing Ghostery preload package. |
| src/plugins/adblocker/.gitignore | Ignores adblock engine binary artifact. |
| src/menu.ts | Creates a unified “Downloader” parent menu combining two downloader plugins. |
| src/i18n/resources/en.json | Adds downloader menu strings + downloader-ytdlp name/description. |
| src/i18n/resources/ar.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/bg.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/bn.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ca.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/cs.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/de.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/el.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/es.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/fa.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/fi.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/fil.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/fr.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/hi.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/hr.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/hu.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/id.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/is.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/it.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ja.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ko.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/lt.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/lv.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ms.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/nb.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ne.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/nl.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/pl.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/pt.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/pt-BR.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ro.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ru.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/sk.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/sl.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/sr.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/sv.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/ta.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/th.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/tr.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/uk.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/vi.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/zh-CN.json | Adds downloader-ytdlp name/description. |
| src/i18n/resources/zh-TW.json | Adds downloader-ytdlp name/description. |
| package.json | Adds chalk + @types/chalk dependencies. |
| README.md | Adds fork-specific documentation content. |
| .github/workflows/build.yml | Adds workflow concurrency configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@total-typescript/ts-reset": "0.6.1", | ||
| "@types/chalk": "^2.2.4", | ||
| "@types/electron-localshortcut": "3.1.3", | ||
| "@types/howler": "2.2.12", |
There was a problem hiding this comment.
@types/chalk is typically for older Chalk versions and can conflict with Chalk v5+, which ships its own types (and is ESM-only). If Chalk is kept, prefer relying on its bundled types and remove @types/chalk to avoid type resolution issues.
| // IDs that will be merged into a single "Downloader" parent menu | ||
| const downloaderIds = ['downloader', 'downloader-ytdlp'] as const; | ||
| const downloaderLabels: Record<string, string> = { | ||
| 'downloader': 'youtube.js (built-in)', | ||
| 'downloader-ytdlp': 'ytdlp (external exe)', | ||
| }; |
There was a problem hiding this comment.
These submenu labels are hard-coded English strings, while the rest of the menu uses t(...) for localization. Consider adding i18n keys for these labels (or deriving them from the plugin’s localized name/description) so the unified Downloader menu is fully localized.
| // Standart YouTube artwork width with margins from both sides is 280 + 720 + 280 | ||
| if (imageSize.width === 1280 && imageSize.height === 720) { |
There was a problem hiding this comment.
Typo in comment: “Standart” should be “Standard”.
| export interface YouTubeFormat { | ||
| itag: number; | ||
| container: string; | ||
| content: string; | ||
| resolution: string; | ||
| bitrate: string; | ||
| range: string; | ||
| vrOr3D: string; | ||
| } | ||
|
|
||
| // converted from https://gist.github.com/sidneys/7095afe4da4ae58694d128b1034e01e2#file-youtube_format_code_itag_list-md | ||
| // and https://gist.github.com/MartinEesmaa/2f4b261cb90a47e9c41ba115a011a4aa | ||
| export const YoutubeFormatList: YouTubeFormat[] = [ | ||
| { | ||
| itag: 5, |
There was a problem hiding this comment.
YoutubeFormatList is a very large constant but isn’t referenced anywhere outside this file. Keeping it in the same module that is imported for presets risks unnecessary bundle/parse cost and makes the file harder to maintain. Consider removing it until needed, or moving it to a separate lazily-imported module.
| "bgutils-js": "3.2.0", | ||
| "butterchurn": "3.0.0-beta.5", | ||
| "butterchurn-presets": "3.0.0-beta.4", | ||
| "chalk": "^5.6.2", |
There was a problem hiding this comment.
chalk is added as a dependency but there are no imports/usages in the codebase changes. If it’s not needed, please remove it to avoid increasing the app bundle and dependency surface area.
| "chalk": "^5.6.2", |
| export const onConfigChange = (newConfig: DownloaderPluginConfig) => { | ||
| config = newConfig; | ||
| // Update cache when config changes | ||
| cachedConfig = newConfig; | ||
|
|
||
| // Reset yt-dlp path cache if custom path changed | ||
| if (cachedConfig?.advanced?.ytDlpPath !== newConfig.advanced?.ytDlpPath) { | ||
| cachedYtDlpPath = undefined; | ||
| } |
There was a problem hiding this comment.
onConfigChange assigns cachedConfig = newConfig before comparing against newConfig, so the custom yt-dlp path change check will never trigger and cachedYtDlpPath won’t be reset. Capture the previous path before overwriting the cache (or compare config/cachedConfig prior to assignment) and reset the cache when the value changes.
| export const getFolder = (customFolder?: string) => | ||
| customFolder ?? app.getPath('downloads'); | ||
|
|
There was a problem hiding this comment.
getFolder treats an empty string as a valid folder and returns it (because '' ?? downloadsPath yields ''). Several callers pass '', which can lead to invalid output paths (e.g., writing to /%(... or failing mkdir). Consider treating empty/whitespace-only strings as unset and falling back to app.getPath('downloads').
| const result = dialog.showOpenDialogSync({ | ||
| properties: ['openDirectory', 'createDirectory'], | ||
| defaultPath: getFolder(config.downloadFolder ?? ''), | ||
| }); |
There was a problem hiding this comment.
This call passes '' into getFolder, which currently returns the empty string rather than falling back to the downloads directory. This can break the defaultPath (and can propagate to download paths). Prefer calling getFolder(config.downloadFolder) (without ?? '') once getFolder treats empty as unset.
| const dir = getFolder(config.downloadFolder ?? ''); | ||
| const playlistUrl = `https://music.youtube.com/playlist?list=${playlistId}`; | ||
|
|
There was a problem hiding this comment.
dir is computed via getFolder(config.downloadFolder ?? ''), which will currently evaluate to '' when downloadFolder is unset. That results in invalid -o paths and can cause downloads to be written to the root or fail. Use getFolder(config.downloadFolder) (and/or make getFolder treat empty string as unset).
| .ytmd-menu-item > .yt-simple-endpoint:hover { | ||
| background-color: var(--ytmusic-menu-item-hover-background-color); | ||
| } |
There was a problem hiding this comment.
The hover selector .ytmd-menu-item > .yt-simple-endpoint:hover doesn’t match the rendered DOM: .ytmd-menu-item is applied to the inner icon <div>, not a parent of .yt-simple-endpoint. This rule will never apply. Adjust the selector to target the actual structure (e.g., hover on the anchor, or move .ytmd-menu-item to the wrapper element).
Changes
This PR implements a unified downloader functionality for the YouTube Music desktop app.
Changes included:
Modified files:
Testing
This has been tested locally and is ready for review.
This contribution is from fork: https://github.com/digitalnomad91/youtube-music