feat(downloader): mejorar descargas (concurrencia, reintentos, omitir…#4551
feat(downloader): mejorar descargas (concurrencia, reintentos, omitir…#4551Rocket-Space wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a configurable ChangesParallel Downloads Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant downloadPlaylistItems
participant runWithConcurrencyLimit
participant downloadSongFromId
User->>Menu: Set maxParallelDownloads
Menu->>downloadPlaylistItems: Start playlist download
downloadPlaylistItems->>runWithConcurrencyLimit: Schedule items with concurrency limit
runWithConcurrencyLimit->>downloadSongFromId: Download song (retry up to 3x)
downloadSongFromId-->>runWithConcurrencyLimit: Success or failure
runWithConcurrencyLimit-->>downloadPlaylistItems: Aggregate progress and failures
downloadPlaylistItems->>User: Prompt retry for failed songs
User-->>downloadPlaylistItems: Confirm retry
downloadPlaylistItems->>downloadPlaylistItems: Recursively rerun failed subset
Estimated code review effort: 3 (Moderate) | ~25 minutes Related issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: enhancement, downloader-plugin, i18n Suggested reviewers: None found in the provided context. 🐰 A rabbit taps its paw with glee, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c852e7f226
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const filePath = join(dir, filename); | ||
|
|
||
| if (config.skipExisting && existsSync(filePath)) { | ||
| if (existsSync(filePath)) { |
There was a problem hiding this comment.
When a user has Skip existing files disabled and tries to re-download a song whose target filename already exists (for example to overwrite a corrupt file or use a different preset/tags), this now returns solely because the file exists, so no download or write occurs. The previous behavior gated this early return on config.skipExisting; please restore that check so the menu toggle still controls whether existing files are skipped.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/downloader/main/index.ts`:
- Around line 841-845: The worker wrapper in the scheduler is swallowing all
errors from the concurrent task, which prevents unexpected failures from
reaching the outer downloadPlaylist error handling. Update the concurrent task
handling around fn(item, currentIndex) so it does not catch every exception
here; let non-song-specific failures propagate naturally, while keeping the
existing per-song failure handling elsewhere intact.
- Around line 471-475: The existing-file early return in the downloader logic
ignores config.skipExisting, so files are being treated as completed duplicates
even when skipping is disabled. Update the existing-file branch in the same area
as the file download flow to only return early when config.skipExisting is true,
and otherwise continue with the normal download path; keep the behavior aligned
with the playlist-folder check and the surrounding file-handling logic in
index.ts.
- Around line 923-932: The retry-failed dialog in the downloader flow is
hard-coded in Spanish instead of using the existing localization system. Update
the dialog in the failedSongs block within the downloader main index logic to
use t(...) for the title, buttons, message, and detail text so it matches the
surrounding user-facing dialogs. Keep the failedSongs mapping and
dialog.showMessageBoxSync flow, but replace the literal strings with
localization keys and interpolated values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a84acbc-dc8f-410d-9940-7903e646287a
📒 Files selected for processing (5)
src/i18n/resources/en.jsonsrc/i18n/resources/es.jsonsrc/plugins/downloader/index.tssrc/plugins/downloader/main/index.tssrc/plugins/downloader/menu.ts
| if (existsSync(filePath)) { | ||
| sendFeedback(null, -1); | ||
| increasePlaylistProgress(1.0); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Respect config.skipExisting before skipping existing files.
Line 471 now treats every existing file as a completed duplicate, even when config.skipExisting is false. That makes the file-level behavior inconsistent with the playlist-folder check at Lines 761-771 and can silently skip requested downloads.
Proposed fix
- if (existsSync(filePath)) {
+ if (config.skipExisting && existsSync(filePath)) {
sendFeedback(null, -1);
increasePlaylistProgress(1.0);
return;
}📝 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.
| if (existsSync(filePath)) { | |
| sendFeedback(null, -1); | |
| increasePlaylistProgress(1.0); | |
| return; | |
| } | |
| if (config.skipExisting && existsSync(filePath)) { | |
| sendFeedback(null, -1); | |
| increasePlaylistProgress(1.0); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/downloader/main/index.ts` around lines 471 - 475, The
existing-file early return in the downloader logic ignores config.skipExisting,
so files are being treated as completed duplicates even when skipping is
disabled. Update the existing-file branch in the same area as the file download
flow to only return early when config.skipExisting is true, and otherwise
continue with the normal download path; keep the behavior aligned with the
playlist-folder check and the surrounding file-handling logic in index.ts.
| try { | ||
| await fn(item, currentIndex); | ||
| } catch (e) { | ||
| console.error('Error in concurrent task:', e); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not swallow unexpected worker failures in the scheduler.
Per-song download failures are already captured in Lines 898-909. Catching every worker error here hides unexpected failures from the outer downloadPlaylist error path.
Proposed fix
- try {
- await fn(item, currentIndex);
- } catch (e) {
- console.error('Error in concurrent task:', e);
- }
+ await fn(item, currentIndex);📝 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.
| try { | |
| await fn(item, currentIndex); | |
| } catch (e) { | |
| console.error('Error in concurrent task:', e); | |
| } | |
| await fn(item, currentIndex); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/downloader/main/index.ts` around lines 841 - 845, The worker
wrapper in the scheduler is swallowing all errors from the concurrent task,
which prevents unexpected failures from reaching the outer downloadPlaylist
error handling. Update the concurrent task handling around fn(item,
currentIndex) so it does not catch every exception here; let non-song-specific
failures propagate naturally, while keeping the existing per-song failure
handling elsewhere intact.
| if (failedSongs.length > 0) { | ||
| const failedListText = failedSongs.map(s => `- ${s.authorName} - ${s.title}`).join('\n'); | ||
| const response = dialog.showMessageBoxSync(win, { | ||
| type: 'warning', | ||
| buttons: ['Reintentar', 'Cancelar'], | ||
| defaultId: 0, | ||
| title: 'Descargas fallidas', | ||
| message: `Las siguientes canciones fallaron después de 3 intentos:`, | ||
| detail: `${failedListText}\n\n¿Desea reintentar la descarga de estas canciones?`, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the retry-failed dialog text.
This user-facing dialog is hard-coded in Spanish, unlike the surrounding downloader dialogs that use t(...).
Proposed direction
- buttons: ['Reintentar', 'Cancelar'],
+ buttons: [
+ t('plugins.downloader.backend.dialog.failed-downloads.buttons.retry'),
+ t('plugins.downloader.backend.dialog.failed-downloads.buttons.cancel'),
+ ],
defaultId: 0,
- title: 'Descargas fallidas',
- message: `Las siguientes canciones fallaron después de 3 intentos:`,
- detail: `${failedListText}\n\n¿Desea reintentar la descarga de estas canciones?`,
+ title: t('plugins.downloader.backend.dialog.failed-downloads.title'),
+ message: t('plugins.downloader.backend.dialog.failed-downloads.message'),
+ detail: t('plugins.downloader.backend.dialog.failed-downloads.detail', {
+ failedListText,
+ }),📝 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.
| if (failedSongs.length > 0) { | |
| const failedListText = failedSongs.map(s => `- ${s.authorName} - ${s.title}`).join('\n'); | |
| const response = dialog.showMessageBoxSync(win, { | |
| type: 'warning', | |
| buttons: ['Reintentar', 'Cancelar'], | |
| defaultId: 0, | |
| title: 'Descargas fallidas', | |
| message: `Las siguientes canciones fallaron después de 3 intentos:`, | |
| detail: `${failedListText}\n\n¿Desea reintentar la descarga de estas canciones?`, | |
| }); | |
| if (failedSongs.length > 0) { | |
| const failedListText = failedSongs.map(s => `- ${s.authorName} - ${s.title}`).join('\n'); | |
| const response = dialog.showMessageBoxSync(win, { | |
| type: 'warning', | |
| buttons: [ | |
| t('plugins.downloader.backend.dialog.failed-downloads.buttons.retry'), | |
| t('plugins.downloader.backend.dialog.failed-downloads.buttons.cancel'), | |
| ], | |
| defaultId: 0, | |
| title: t('plugins.downloader.backend.dialog.failed-downloads.title'), | |
| message: t('plugins.downloader.backend.dialog.failed-downloads.message'), | |
| detail: t('plugins.downloader.backend.dialog.failed-downloads.detail', { | |
| failedListText, | |
| }), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/downloader/main/index.ts` around lines 923 - 932, The
retry-failed dialog in the downloader flow is hard-coded in Spanish instead of
using the existing localization system. Update the dialog in the failedSongs
block within the downloader main index logic to use t(...) for the title,
buttons, message, and detail text so it matches the surrounding user-facing
dialogs. Keep the failedSongs mapping and dialog.showMessageBoxSync flow, but
replace the literal strings with localization keys and interpolated values.
1 participant
… duplicados)
Summary by CodeRabbit
New Features
Bug Fixes