various: replace yt-dlp with custom implementation - #616
Open
temidaradev wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughNative YouTube Music downloads replace the yt-dlp workflow. The change adds native search and stream resolution, download jobs, format conversion, metadata and artwork handling, updated configuration and routing, localized UI strings, documentation, and packaging dependencies. ChangesNative YouTube download flow
Sequence Diagram(s)sequenceDiagram
participant User
participant YoutubeDownloadsPage
participant YoutubeDownloadClient
participant YoutubeDownloadJobs
participant ffmpeg
User->>YoutubeDownloadsPage: Enter search query
YoutubeDownloadsPage->>YoutubeDownloadClient: search(query)
YoutubeDownloadClient-->>YoutubeDownloadsPage: Track results
User->>YoutubeDownloadsPage: Start download
YoutubeDownloadsPage->>YoutubeDownloadJobs: start_download(track, options)
YoutubeDownloadJobs->>YoutubeDownloadClient: resolve_stream(video_id)
YoutubeDownloadClient-->>YoutubeDownloadJobs: Stream information
YoutubeDownloadJobs->>ffmpeg: Convert and embed metadata when required
ffmpeg-->>YoutubeDownloadJobs: Output file
YoutubeDownloadJobs-->>YoutubeDownloadsPage: Progress and completion status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packaging/flatpak/com.temidaradev.kopuz.json (1)
40-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
ffmpegavailable to the Flatpak sandbox.
start_download()requiresffmpegwhen conversion, metadata, or embedded artwork is requested, but the Flatpak manifest has noffmpegdependency. Add anffmpegmodule or the matching Flatpak extension/SDK extension so these options do not fail inside the app sandbox.🤖 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 `@packaging/flatpak/com.temidaradev.kopuz.json` at line 40, Add an ffmpeg module or compatible Flatpak/SDK extension to the manifest module list in com.temidaradev.kopuz.json, ensuring the ffmpeg executable is available inside the sandbox for start_download() conversion, metadata, and embedded-artwork options.
🧹 Nitpick comments (3)
crates/i18n/locales/fil.ftl (1)
104-147: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftComplete the YouTube download translations in all supplied locales.
The new block is only partially localized. Most user-facing values remain English in each supplied non-English locale. Translate the messages while preserving Fluent keys and variable references.
crates/i18n/locales/fil.ftl#L104-L147: Translate the Filipino YouTube download messages.crates/i18n/locales/fr.ftl#L87-L130: Translate the French YouTube download messages.crates/i18n/locales/gr.ftl#L87-L130: Translate the Greek YouTube download messages.crates/i18n/locales/he.ftl#L87-L130: Translate the Hebrew YouTube download messages.crates/i18n/locales/hu.ftl#L87-L130: Translate the Hungarian YouTube download messages.crates/i18n/locales/tr.ftl#L87-L130: Translate the Turkish YouTube download messages.crates/i18n/locales/uk.ftl#L87-L130: Translate the Ukrainian YouTube download messages.crates/i18n/locales/vi-VN.ftl#L104-L147: Translate the Vietnamese YouTube download messages.crates/i18n/locales/zh-CN.ftl#L87-L130: Translate the Simplified Chinese YouTube download messages.🤖 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 `@crates/i18n/locales/fil.ftl` around lines 104 - 147, Translate every user-facing value in the YouTube download message block, preserving all Fluent keys and variable references. Apply the appropriate Filipino translations in crates/i18n/locales/fil.ftl lines 104-147, French in crates/i18n/locales/fr.ftl lines 87-130, Greek in crates/i18n/locales/gr.ftl lines 87-130, Hebrew in crates/i18n/locales/he.ftl lines 87-130, Hungarian in crates/i18n/locales/hu.ftl lines 87-130, Turkish in crates/i18n/locales/tr.ftl lines 87-130, Ukrainian in crates/i18n/locales/uk.ftl lines 87-130, Vietnamese in crates/i18n/locales/vi-VN.ftl lines 104-147, and Simplified Chinese in crates/i18n/locales/zh-CN.ftl lines 87-130; ensure no English source text remains in these localized blocks.crates/pages/src/youtube_downloads.rs (1)
457-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA truncated failure message hides the reason for the failure.
Line 459 applies
truncateto the status paragraph. ForJobStatus::Failed, line 437 renders the raw error message, which can be long, for example the ffmpeg stderr text produced atcrates/pages/src/youtube_download_jobs.rsline 705. The user sees a clipped line and has no way to read the rest.Add a
titleattribute so the full text is available on hover.♻️ Proposed fix
p { class: if matches!(&job.status, JobStatus::Failed(_)) { "text-red-400 text-xs mt-0.5 truncate" } else { "text-slate-500 text-xs mt-0.5" }, + title: "{status}", "{status}" }🤖 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 `@crates/pages/src/youtube_downloads.rs` around lines 457 - 464, Add a title attribute to the status paragraph in the JobStatus rendering block so it contains the full status text, including untruncated failure messages, while preserving the existing truncate styling and status display.crates/config/src/lib.rs (1)
1155-1185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
organize_by_albumdefault in the legacy migration test.The legacy JSON omits
organize_by_album. That field is the only option with a non-falseserde default (default_true). The test does not cover it, so a future change to the#[serde(default = "default_true")]attribute would silently flip every migrated config tofalsewithout failing this test.♻️ Proposed additional assertion
assert!(config.youtube_download_options.write_thumbnail); + assert!(config.youtube_download_options.organize_by_album); + assert!(!config.youtube_download_options.overwrite_existing); assert_eq!(config.youtube_download_history.len(), 1);🤖 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 `@crates/config/src/lib.rs` around lines 1155 - 1185, Extend legacy_downloader_config_migrates_to_native_youtube_fields to assert that config.youtube_download_options.organize_by_album remains true when the legacy JSON omits the field, covering its default_true serde behavior.
🤖 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 `@crates/i18n/locales/ar.ftl`:
- Around line 87-130: Translate the complete youtube_download_* message block,
preserving every Fluent key and variable, in crates/i18n/locales/ar.ftl lines
87-130, de.ftl lines 87-130, es.ftl lines 87-130, ro.ftl lines 87-130, ru.ftl
lines 87-130, sv.ftl lines 104-147, ta.ftl lines 104-147, tok-SP.ftl lines
87-130, and tok.ftl lines 87-130 using each locale’s native language or writing
system.
In `@crates/i18n/locales/id.ftl`:
- Around line 104-147: Translate every new YouTube download message in
crates/i18n/locales/id.ftl:104-147 into Indonesian;
crates/i18n/locales/it.ftl:104-147 into Italian;
crates/i18n/locales/ja.ftl:87-130 into Japanese;
crates/i18n/locales/ko.ftl:87-130 into Korean;
crates/i18n/locales/ml.ftl:104-147 into Malayalam;
crates/i18n/locales/nl.ftl:104-147 into Dutch; crates/i18n/locales/pl.ftl:87-130
into Polish; crates/i18n/locales/pt-BR.ftl:87-130 into Brazilian Portuguese; and
crates/i18n/locales/pt-PT.ftl:87-130 into European Portuguese. Preserve every
Fluent key, variable such as $percent, $speed, $eta, $error, and $path, format
identifiers, and the intended UI meaning.
In `@crates/pages/src/youtube_download_jobs.rs`:
- Around line 156-180: Make run_preflight_checks asynchronous from the download
button flow by moving its blocking duplicate-job, find_binary, and
validate_output_directory work into tokio::task::spawn_blocking. Await the
preflight result inside the existing spawn in the download click handler,
preserving the current validation errors and success behavior without blocking
the UI thread.
- Around line 563-584: Update the cover download flow around reqwest::get in
download_cover to use the existing bounded HTTP-client pattern, including
connect and total request timeouts. Enforce a maximum response body size before
collecting bytes, preserving the current success handling and warning/None
behavior for request and body failures.
- Around line 332-339: Track destination ownership in the download flow around
destination_path and the result error handling: record whether this job
successfully created or replaced the destination, and only remove destination on
failure when that flag is true. Preserve existing cleanup for source_path and
cover, while ensuring overwrite_existing failures never delete a pre-existing
user file.
In `@crates/pages/src/youtube_downloads.rs`:
- Around line 301-315: Update the button rendered by OptionToggle to include an
aria-pressed attribute bound to props.enabled, so assistive technology exposes
the toggle’s current state while preserving the existing click behavior and
styling.
- Around line 134-139: Remove the per-keystroke config mutation from the oninput
handler in the YouTube downloads form, keeping only page-error clearing, value
reading, and out_dir updates. Persist youtube_download_output_dir in an onblur
handler when the field loses focus, while leaving the folder picker’s existing
discrete config write unchanged.
---
Outside diff comments:
In `@packaging/flatpak/com.temidaradev.kopuz.json`:
- Line 40: Add an ffmpeg module or compatible Flatpak/SDK extension to the
manifest module list in com.temidaradev.kopuz.json, ensuring the ffmpeg
executable is available inside the sandbox for start_download() conversion,
metadata, and embedded-artwork options.
---
Nitpick comments:
In `@crates/config/src/lib.rs`:
- Around line 1155-1185: Extend
legacy_downloader_config_migrates_to_native_youtube_fields to assert that
config.youtube_download_options.organize_by_album remains true when the legacy
JSON omits the field, covering its default_true serde behavior.
In `@crates/i18n/locales/fil.ftl`:
- Around line 104-147: Translate every user-facing value in the YouTube download
message block, preserving all Fluent keys and variable references. Apply the
appropriate Filipino translations in crates/i18n/locales/fil.ftl lines 104-147,
French in crates/i18n/locales/fr.ftl lines 87-130, Greek in
crates/i18n/locales/gr.ftl lines 87-130, Hebrew in crates/i18n/locales/he.ftl
lines 87-130, Hungarian in crates/i18n/locales/hu.ftl lines 87-130, Turkish in
crates/i18n/locales/tr.ftl lines 87-130, Ukrainian in crates/i18n/locales/uk.ftl
lines 87-130, Vietnamese in crates/i18n/locales/vi-VN.ftl lines 104-147, and
Simplified Chinese in crates/i18n/locales/zh-CN.ftl lines 87-130; ensure no
English source text remains in these localized blocks.
In `@crates/pages/src/youtube_downloads.rs`:
- Around line 457-464: Add a title attribute to the status paragraph in the
JobStatus rendering block so it contains the full status text, including
untruncated failure messages, while preserving the existing truncate styling and
status display.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e1696255-261f-4f14-ba53-88d78106267b
📒 Files selected for processing (53)
.github/ISSUE_TEMPLATE/feature_request.yml.github/ISSUE_TEMPLATE/performance_report.ymlCONTRIBUTING.mdREADME.mdcrates/components/src/normal/sidebar.rscrates/components/src/vaxry/sidebar.rscrates/config/src/lib.rscrates/i18n/locales/ar.ftlcrates/i18n/locales/de.ftlcrates/i18n/locales/en.ftlcrates/i18n/locales/es.ftlcrates/i18n/locales/fil.ftlcrates/i18n/locales/fr.ftlcrates/i18n/locales/gr.ftlcrates/i18n/locales/he.ftlcrates/i18n/locales/hu.ftlcrates/i18n/locales/id.ftlcrates/i18n/locales/it.ftlcrates/i18n/locales/ja.ftlcrates/i18n/locales/ko.ftlcrates/i18n/locales/ml.ftlcrates/i18n/locales/nl.ftlcrates/i18n/locales/pl.ftlcrates/i18n/locales/pt-BR.ftlcrates/i18n/locales/pt-PT.ftlcrates/i18n/locales/ro.ftlcrates/i18n/locales/ru.ftlcrates/i18n/locales/sv.ftlcrates/i18n/locales/ta.ftlcrates/i18n/locales/tok-SP.ftlcrates/i18n/locales/tok.ftlcrates/i18n/locales/tr.ftlcrates/i18n/locales/uk.ftlcrates/i18n/locales/vi-VN.ftlcrates/i18n/locales/zh-CN.ftlcrates/kopuz/src/main.rscrates/kopuz_route/src/lib.rscrates/pages/Cargo.tomlcrates/pages/src/lib.rscrates/pages/src/youtube_download_jobs.rscrates/pages/src/youtube_downloads.rscrates/pages/src/ytdlp.rscrates/pages/src/ytdlp_jobs.rscrates/server/src/lib.rscrates/server/src/youtube_download.rsdocs/README-ML.mddocs/README-PT-PT.mddocs/README-TR.mdpackaging/aur/PKGBUILDpackaging/aur/PKGBUILD-binpackaging/flatpak/com.temidaradev.kopuz.jsonpackaging/nix/crane.nixpackaging/nix/shell.nix
💤 Files with no reviewable changes (2)
- crates/pages/src/ytdlp.rs
- crates/pages/src/ytdlp_jobs.rs
Comment on lines
+87
to
+130
| youtube_downloads = التنزيلات | ||
| youtube_download_title = YouTube Downloads | ||
| youtube_download_subtitle = Search YouTube Music and download with Kopuz | ||
| youtube_download_options = Options | ||
| youtube_download_search_placeholder = Song title, artist, or album… | ||
| youtube_download_search = Search | ||
| youtube_download_searching = Searching… | ||
| youtube_download_results = Search results | ||
| youtube_download_download = Download | ||
| youtube_download_output_dir_placeholder = Output directory (defaults to music folder) | ||
| youtube_download_pick_folder = Pick folder | ||
| youtube_download_clear_history = Clear history | ||
| youtube_download_empty_state = Search for a song to start | ||
| youtube_download_no_results = No songs found | ||
| youtube_download_jobs = Downloads | ||
| youtube_download_file_options = File options | ||
| youtube_download_embed_metadata = Embed metadata | ||
| youtube_download_embed_metadata_desc = Save title, artist, album, and source URL in the audio file | ||
| youtube_download_embed_thumbnail = Embed cover artwork | ||
| youtube_download_embed_thumbnail_desc = Embed when the format supports it; otherwise save a sidecar JPG | ||
| youtube_download_write_thumbnail = Save cover separately | ||
| youtube_download_write_thumbnail_desc = Write a JPG beside the downloaded audio | ||
| youtube_download_organize_album = Organize by album | ||
| youtube_download_organize_album_desc = Put downloads in an album-named folder | ||
| youtube_download_overwrite = Overwrite existing files | ||
| youtube_download_overwrite_desc = Replace a file with the same output name | ||
| youtube_download_ffmpeg_hint = Original downloads need no external downloader. ffmpeg is only required for conversion, metadata, or embedded artwork. | ||
| youtube_download_format_original = Original quality | ||
| youtube_download_format_mp3 = MP3 | ||
| youtube_download_format_flac = FLAC | ||
| youtube_download_format_opus = OPUS | ||
| youtube_download_format_wav = WAV | ||
| youtube_download_status_resolving = Resolving native YouTube stream… | ||
| youtube_download_status_downloading_eta = { $percent }% { $speed } ETA { $eta } | ||
| youtube_download_status_downloading = { $percent }% | ||
| youtube_download_status_processing = Processing… | ||
| youtube_download_status_completed = Completed | ||
| youtube_download_status_waiting = Waiting… | ||
| youtube_download_error_search = Search failed: { $error } | ||
| youtube_download_error_ffmpeg_required = ffmpeg is required for this format or the selected file options. Install ffmpeg or choose Original quality and disable embedding. | ||
| youtube_download_error_duplicate_active = This song is already downloading. | ||
| youtube_download_error_output_not_directory = Output path is not a directory: { $path } | ||
| youtube_download_error_output_prepare = Failed to prepare output directory: { $error } | ||
| youtube_download_error_output_not_writable = Output directory is not writable: { $path } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the native download block in all affected locales.
The new youtube_download_* messages are copied in English across these non-English locale files. This makes the native download workflow appear untranslated.
crates/i18n/locales/ar.ftl#L87-L130: add Arabic translations.crates/i18n/locales/de.ftl#L87-L130: add German translations.crates/i18n/locales/es.ftl#L87-L130: add Spanish translations.crates/i18n/locales/ro.ftl#L87-L130: add Romanian translations.crates/i18n/locales/ru.ftl#L87-L130: add Russian translations.crates/i18n/locales/sv.ftl#L104-L147: add Swedish translations.crates/i18n/locales/ta.ftl#L104-L147: add Tamil translations.crates/i18n/locales/tok-SP.ftl#L87-L130: addsitelen ponatranslations.crates/i18n/locales/tok.ftl#L87-L130: add toki pona translations.
Preserve all Fluent keys and variables.
📍 Affects 9 files
crates/i18n/locales/ar.ftl#L87-L130(this comment)crates/i18n/locales/de.ftl#L87-L130crates/i18n/locales/es.ftl#L87-L130crates/i18n/locales/ro.ftl#L87-L130crates/i18n/locales/ru.ftl#L87-L130crates/i18n/locales/sv.ftl#L104-L147crates/i18n/locales/ta.ftl#L104-L147crates/i18n/locales/tok-SP.ftl#L87-L130crates/i18n/locales/tok.ftl#L87-L130
🤖 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 `@crates/i18n/locales/ar.ftl` around lines 87 - 130, Translate the complete
youtube_download_* message block, preserving every Fluent key and variable, in
crates/i18n/locales/ar.ftl lines 87-130, de.ftl lines 87-130, es.ftl lines
87-130, ro.ftl lines 87-130, ru.ftl lines 87-130, sv.ftl lines 104-147, ta.ftl
lines 104-147, tok-SP.ftl lines 87-130, and tok.ftl lines 87-130 using each
locale’s native language or writing system.
Comment on lines
+104
to
+147
| youtube_downloads = Unduhan | ||
| youtube_download_title = YouTube Downloads | ||
| youtube_download_subtitle = Search YouTube Music and download with Kopuz | ||
| youtube_download_options = Options | ||
| youtube_download_search_placeholder = Song title, artist, or album… | ||
| youtube_download_search = Search | ||
| youtube_download_searching = Searching… | ||
| youtube_download_results = Search results | ||
| youtube_download_download = Download | ||
| youtube_download_output_dir_placeholder = Output directory (defaults to music folder) | ||
| youtube_download_pick_folder = Pick folder | ||
| youtube_download_clear_history = Clear history | ||
| youtube_download_empty_state = Search for a song to start | ||
| youtube_download_no_results = No songs found | ||
| youtube_download_jobs = Downloads | ||
| youtube_download_file_options = File options | ||
| youtube_download_embed_metadata = Embed metadata | ||
| youtube_download_embed_metadata_desc = Save title, artist, album, and source URL in the audio file | ||
| youtube_download_embed_thumbnail = Embed cover artwork | ||
| youtube_download_embed_thumbnail_desc = Embed when the format supports it; otherwise save a sidecar JPG | ||
| youtube_download_write_thumbnail = Save cover separately | ||
| youtube_download_write_thumbnail_desc = Write a JPG beside the downloaded audio | ||
| youtube_download_organize_album = Organize by album | ||
| youtube_download_organize_album_desc = Put downloads in an album-named folder | ||
| youtube_download_overwrite = Overwrite existing files | ||
| youtube_download_overwrite_desc = Replace a file with the same output name | ||
| youtube_download_ffmpeg_hint = Original downloads need no external downloader. ffmpeg is only required for conversion, metadata, or embedded artwork. | ||
| youtube_download_format_original = Original quality | ||
| youtube_download_format_mp3 = MP3 | ||
| youtube_download_format_flac = FLAC | ||
| youtube_download_format_opus = OPUS | ||
| youtube_download_format_wav = WAV | ||
| youtube_download_status_resolving = Resolving native YouTube stream… | ||
| youtube_download_status_downloading_eta = { $percent }% { $speed } ETA { $eta } | ||
| youtube_download_status_downloading = { $percent }% | ||
| youtube_download_status_processing = Processing… | ||
| youtube_download_status_completed = Completed | ||
| youtube_download_status_waiting = Waiting… | ||
| youtube_download_error_search = Search failed: { $error } | ||
| youtube_download_error_ffmpeg_required = ffmpeg is required for this format or the selected file options. Install ffmpeg or choose Original quality and disable embedding. | ||
| youtube_download_error_duplicate_active = This song is already downloading. | ||
| youtube_download_error_output_not_directory = Output path is not a directory: { $path } | ||
| youtube_download_error_output_prepare = Failed to prepare output directory: { $error } | ||
| youtube_download_error_output_not_writable = Output directory is not writable: { $path } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new YouTube download strings in every locale.
The replacement block uses English values in all nine supplied non-English locale files. This leaves the new download UI untranslated. Translate each value while preserving the Fluent keys and Fluent variables.
crates/i18n/locales/id.ftl#L104-L147: Translate the new YouTube download values into Indonesian.crates/i18n/locales/it.ftl#L104-L147: Translate the new YouTube download values into Italian.crates/i18n/locales/ja.ftl#L87-L130: Translate the new YouTube download values into Japanese.crates/i18n/locales/ko.ftl#L87-L130: Translate the new YouTube download values into Korean.crates/i18n/locales/ml.ftl#L104-L147: Translate the new YouTube download values into Malayalam.crates/i18n/locales/nl.ftl#L104-L147: Translate the new YouTube download values into Dutch.crates/i18n/locales/pl.ftl#L87-L130: Translate the new YouTube download values into Polish.crates/i18n/locales/pt-BR.ftl#L87-L130: Translate the new YouTube download values into Brazilian Portuguese.crates/i18n/locales/pt-PT.ftl#L87-L130: Translate the new YouTube download values into European Portuguese.
📍 Affects 9 files
crates/i18n/locales/id.ftl#L104-L147(this comment)crates/i18n/locales/it.ftl#L104-L147crates/i18n/locales/ja.ftl#L87-L130crates/i18n/locales/ko.ftl#L87-L130crates/i18n/locales/ml.ftl#L104-L147crates/i18n/locales/nl.ftl#L104-L147crates/i18n/locales/pl.ftl#L87-L130crates/i18n/locales/pt-BR.ftl#L87-L130crates/i18n/locales/pt-PT.ftl#L87-L130
🤖 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 `@crates/i18n/locales/id.ftl` around lines 104 - 147, Translate every new
YouTube download message in crates/i18n/locales/id.ftl:104-147 into Indonesian;
crates/i18n/locales/it.ftl:104-147 into Italian;
crates/i18n/locales/ja.ftl:87-130 into Japanese;
crates/i18n/locales/ko.ftl:87-130 into Korean;
crates/i18n/locales/ml.ftl:104-147 into Malayalam;
crates/i18n/locales/nl.ftl:104-147 into Dutch; crates/i18n/locales/pl.ftl:87-130
into Polish; crates/i18n/locales/pt-BR.ftl:87-130 into Brazilian Portuguese; and
crates/i18n/locales/pt-PT.ftl:87-130 into European Portuguese. Preserve every
Fluent key, variable such as $percent, $speed, $eta, $error, and $path, format
identifiers, and the intended UI meaning.
Comment on lines
+156
to
+180
| pub fn run_preflight_checks( | ||
| video_id: &str, | ||
| out_dir: &str, | ||
| format: AudioFormat, | ||
| options: &YoutubeDownloadOptions, | ||
| ) -> Result<(), String> { | ||
| if JOBS.read().iter().any(|job| { | ||
| job.video_id == video_id | ||
| && matches!( | ||
| job.status, | ||
| JobStatus::Pending | ||
| | JobStatus::Resolving | ||
| | JobStatus::Downloading | ||
| | JobStatus::Processing | ||
| ) | ||
| }) { | ||
| return Err(i18n::t("youtube_download_error_duplicate_active")); | ||
| } | ||
|
|
||
| if requires_ffmpeg(format, options) && find_binary("ffmpeg").is_none() { | ||
| return Err(i18n::t("youtube_download_error_ffmpeg_required")); | ||
| } | ||
|
|
||
| validate_output_directory(&output_root(out_dir)) | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
run_preflight_checks performs blocking work on the UI thread.
run_preflight_checks runs synchronously from the download button handler in crates/pages/src/youtube_downloads.rs line 181. It performs two blocking operations:
- Line 175 calls
find_binary, which callssearch_dirs. On the first call of the process,search_dirs(lines 879-884) spawns a login shell ($SHELL -lc) and waits for its output. A user's shell profile can take hundreds of milliseconds or longer to initialize. - Line 179 calls
validate_output_directory, which performs blockingfs::create_dir_all, a file create probe, and a file delete. On a network mount or a slow external volume, these block for seconds.
Both stall the render thread, so the window freezes on the first download click.
Consider running the preflight checks on a blocking worker and making the caller async, for example by moving the body into tokio::task::spawn_blocking and awaiting it inside the existing spawn in the click handler.
🤖 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 `@crates/pages/src/youtube_download_jobs.rs` around lines 156 - 180, Make
run_preflight_checks asynchronous from the download button flow by moving its
blocking duplicate-job, find_binary, and validate_output_directory work into
tokio::task::spawn_blocking. Await the preflight result inside the existing
spawn in the download click handler, preserving the current validation errors
and success behavior without blocking the UI thread.
Comment on lines
+332
to
+339
| let _ = tokio::fs::remove_file(&source_path).await; | ||
| if let Err(error) = result { | ||
| let _ = tokio::fs::remove_file(&destination).await; | ||
| if let Some(path) = cover { | ||
| let _ = tokio::fs::remove_file(path).await; | ||
| } | ||
| return Err(error); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The error path can delete a pre-existing user file.
When options.overwrite_existing is true, destination_path (line 774) returns the wanted path unchanged, so destination can point at a file that already existed before this job started.
If the processing step then fails, line 334 deletes destination. That removes a user file which this download never created and never successfully replaced. A failed download must not destroy the previous copy.
Track whether this job actually created the destination, and delete it only in that case.
🛡️ Proposed fix
let stem = output_stem(track);
let process = requires_ffmpeg(format, options);
let extension = output_extension(format, stream.format, process);
let wanted = destination_dir.join(format!("{stem}.{extension}"));
let destination = destination_path(&wanted, options.overwrite_existing);
+ // Only a file this job created may be removed on failure; an
+ // overwrite target that predates the job must survive a failed download.
+ let destination_preexisted = destination.exists(); let _ = tokio::fs::remove_file(&source_path).await;
if let Err(error) = result {
- let _ = tokio::fs::remove_file(&destination).await;
+ if !destination_preexisted {
+ let _ = tokio::fs::remove_file(&destination).await;
+ }
if let Some(path) = cover {
let _ = tokio::fs::remove_file(path).await;
}
return Err(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.
Suggested change
| let _ = tokio::fs::remove_file(&source_path).await; | |
| if let Err(error) = result { | |
| let _ = tokio::fs::remove_file(&destination).await; | |
| if let Some(path) = cover { | |
| let _ = tokio::fs::remove_file(path).await; | |
| } | |
| return Err(error); | |
| } | |
| let _ = tokio::fs::remove_file(&source_path).await; | |
| if let Err(error) = result { | |
| if !destination_preexisted { | |
| let _ = tokio::fs::remove_file(&destination).await; | |
| } | |
| if let Some(path) = cover { | |
| let _ = tokio::fs::remove_file(path).await; | |
| } | |
| return Err(error); | |
| } |
🤖 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 `@crates/pages/src/youtube_download_jobs.rs` around lines 332 - 339, Track
destination ownership in the download flow around destination_path and the
result error handling: record whether this job successfully created or replaced
the destination, and only remove destination on failure when that flag is true.
Preserve existing cleanup for source_path and cover, while ensuring
overwrite_existing failures never delete a pre-existing user file.
Comment on lines
+563
to
+584
| let response = match reqwest::get(&url).await { | ||
| Ok(response) if response.status().is_success() => response, | ||
| Ok(response) => { | ||
| tracing::warn!( | ||
| target: "youtube_download", | ||
| status = %response.status(), | ||
| "cover download returned an error" | ||
| ); | ||
| return None; | ||
| } | ||
| Err(error) => { | ||
| tracing::warn!(target: "youtube_download", %error, "cover download failed"); | ||
| return None; | ||
| } | ||
| }; | ||
| let bytes = match response.bytes().await { | ||
| Ok(bytes) => bytes, | ||
| Err(error) => { | ||
| tracing::warn!(target: "youtube_download", %error, "cover body failed"); | ||
| return None; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the cover download.
Line 563 uses reqwest::get, which builds a default client with no connect timeout and no total request timeout. download_track awaits download_cover at line 304, between the audio download and the processing step. If the cover host stops responding, the job hangs in Downloading indefinitely, and the user cannot cancel it.
The audio paths already bound every request (lines 365, 388, and 501). Apply the same treatment here. Bound the body size as well, so an oversized response cannot exhaust memory.
🛡️ Proposed fix
- let response = match reqwest::get(&url).await {
+ let client = match reqwest::Client::builder()
+ .connect_timeout(std::time::Duration::from_secs(10))
+ .timeout(std::time::Duration::from_secs(30))
+ .build()
+ {
+ Ok(client) => client,
+ Err(error) => {
+ tracing::warn!(target: "youtube_download", %error, "cover client build failed");
+ return None;
+ }
+ };
+ let response = match client.get(&url).send().await {
Ok(response) if response.status().is_success() => response,🤖 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 `@crates/pages/src/youtube_download_jobs.rs` around lines 563 - 584, Update the
cover download flow around reqwest::get in download_cover to use the existing
bounded HTTP-client pattern, including connect and total request timeouts.
Enforce a maximum response body size before collecting bytes, preserving the
current success handling and warning/None behavior for request and body
failures.
Comment on lines
+134
to
+139
| oninput: move |event| { | ||
| page_error.set(None); | ||
| let value = event.value(); | ||
| out_dir.set(value.clone()); | ||
| config.write().youtube_download_output_dir = value; | ||
| }, |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not write the whole app config on every keystroke.
Line 138 writes config.write().youtube_download_output_dir on each oninput event. config is provided as app-wide context in crates/kopuz/src/main.rs line 1565, so every character typed marks the config dirty (main.rs lines 876-882) and re-renders every subscriber of that signal. The comment at main.rs lines 867-874 records that whole-config writes were expensive enough to starve the audio stream.
out_dir is already a local signal, so the input stays responsive without the config write. Persist the value when the field loses focus instead. The folder picker at line 150 already writes the config at a single discrete moment and needs no change.
♻️ Proposed fix
oninput: move |event| {
page_error.set(None);
- let value = event.value();
- out_dir.set(value.clone());
- config.write().youtube_download_output_dir = value;
+ out_dir.set(event.value());
},
+ onblur: move |_| {
+ config.write().youtube_download_output_dir = out_dir();
+ },
onkeydown: move |event| event.stop_propagation(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| oninput: move |event| { | |
| page_error.set(None); | |
| let value = event.value(); | |
| out_dir.set(value.clone()); | |
| config.write().youtube_download_output_dir = value; | |
| }, | |
| oninput: move |event| { | |
| page_error.set(None); | |
| out_dir.set(event.value()); | |
| }, | |
| onblur: move |_| { | |
| config.write().youtube_download_output_dir = out_dir(); | |
| }, | |
| onkeydown: move |event| event.stop_propagation(), |
🤖 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 `@crates/pages/src/youtube_downloads.rs` around lines 134 - 139, Remove the
per-keystroke config mutation from the oninput handler in the YouTube downloads
form, keeping only page-error clearing, value reading, and out_dir updates.
Persist youtube_download_output_dir in an onblur handler when the field loses
focus, while leaving the folder picker’s existing discrete config write
unchanged.
Comment on lines
+301
to
+315
| fn OptionToggle(props: OptionToggleProps) -> Element { | ||
| rsx! { | ||
| button { | ||
| class: "flex items-start gap-2 py-1.5 text-left group", | ||
| onclick: move |_| props.on_change.call(!props.enabled), | ||
| div { | ||
| class: if props.enabled { | ||
| "w-4 h-4 mt-0.5 rounded border border-white/40 bg-white/20 flex items-center justify-center shrink-0" | ||
| } else { | ||
| "w-4 h-4 mt-0.5 rounded border border-white/15 bg-transparent flex items-center justify-center shrink-0" | ||
| }, | ||
| if props.enabled { | ||
| i { class: "fa-solid fa-check text-white text-[9px]" } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Expose the toggle state to assistive technology.
OptionToggle renders a button whose on/off state is conveyed only by CSS classes and a check glyph. A screen reader announces the label but not whether the option is enabled, so a non-sighted user cannot determine or verify the current setting.
Add aria-pressed bound to props.enabled.
♿ Proposed fix
button {
class: "flex items-start gap-2 py-1.5 text-left group",
+ "aria-pressed": "{props.enabled}",
onclick: move |_| props.on_change.call(!props.enabled),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn OptionToggle(props: OptionToggleProps) -> Element { | |
| rsx! { | |
| button { | |
| class: "flex items-start gap-2 py-1.5 text-left group", | |
| onclick: move |_| props.on_change.call(!props.enabled), | |
| div { | |
| class: if props.enabled { | |
| "w-4 h-4 mt-0.5 rounded border border-white/40 bg-white/20 flex items-center justify-center shrink-0" | |
| } else { | |
| "w-4 h-4 mt-0.5 rounded border border-white/15 bg-transparent flex items-center justify-center shrink-0" | |
| }, | |
| if props.enabled { | |
| i { class: "fa-solid fa-check text-white text-[9px]" } | |
| } | |
| } | |
| fn OptionToggle(props: OptionToggleProps) -> Element { | |
| rsx! { | |
| button { | |
| class: "flex items-start gap-2 py-1.5 text-left group", | |
| "aria-pressed": "{props.enabled}", | |
| onclick: move |_| props.on_change.call(!props.enabled), | |
| div { | |
| class: if props.enabled { | |
| "w-4 h-4 mt-0.5 rounded border border-white/40 bg-white/20 flex items-center justify-center shrink-0" | |
| } else { | |
| "w-4 h-4 mt-0.5 rounded border border-white/15 bg-transparent flex items-center justify-center shrink-0" | |
| }, | |
| if props.enabled { | |
| i { class: "fa-solid fa-check text-white text-[9px]" } | |
| } | |
| } |
🤖 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 `@crates/pages/src/youtube_downloads.rs` around lines 301 - 315, Update the
button rendered by OptionToggle to include an aria-pressed attribute bound to
props.enabled, so assistive technology exposes the toggle’s current state while
preserving the existing click behavior and styling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
uhh yeah so yt-dlp was stupid
Sanity Checking
rules.
contribution guidelines, or this pull request did not use AI assistance.
Style and Consistency
style.
cargo fmt --all --checkorcargo fmt --allas appropriate.cargo clippy --workspace --all-targets -- -D warnings, orexplained why it could not be run.
this change depends on them.
Testing
Tested on platform(s):
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwin