feat(download): YouTube 1080p+ via download_to_file DASH fallback#72
Merged
feat(download): YouTube 1080p+ via download_to_file DASH fallback#72
Conversation
…ct AdaptiveStreamOnly
Introduces RegisterLocalFileCommand and its handler on CommandBus, which registers an already-downloaded local file directly as a Completed download (Queued → Downloading → Completed state transition). Also adds Download::set_file_size to the domain model.
Share a single next_download_id() (pub(super)) between start_download and register_local_file to prevent ID collisions; document set_file_size side effect and the intentional discard of the DownloadStarted event.
…(fixes 1080p YouTube)
New WASM adds yt-dlp workarounds (--extractor-args player_client fallback, --retries, --fragment-retries, --quiet) that fix HTTP 403 Forbidden on SABR-protected YouTube videos (VEVO music, age-gated). New checksums: WASM: 60a77e3f70e9d8ed114af204575cb80cc1c0127f36aca5ff4ce0e537b4fa1739 plugin.toml: 31341508a80f54dc42bc2387771a26feb7a6e90a9191e43f1e6bd5e8d37c98cb
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds YouTube adaptive-stream fallback: when stream resolution returns Changes
Sequence DiagramsequenceDiagram
actor Client
participant IPC as Tauri IPC
participant Loader as Plugin Loader
participant Plugin as YouTube Plugin
participant FS as File System
Client->>IPC: download_media_start(url, title, ...)
IPC->>Loader: resolve_stream_url(url)
Loader->>Plugin: resolve_stream_url(url)
Plugin-->>Loader: Err(AdaptiveStreamOnly)
Loader-->>IPC: Err(AdaptiveStreamOnly)
alt Adaptive-stream fallback
IPC->>Loader: download_to_file(url, quality, format, temp_dir)
Loader->>Plugin: download_to_file(url, quality, format, temp_dir)
Plugin->>FS: download DASH segments + merge (yt-dlp/ffmpeg)
FS-->>Plugin: merged_file_path
Plugin-->>Loader: DownloadedFileInfo { path, size }
Loader-->>IPC: DownloadedFileInfo
IPC->>IPC: sanitize title → filename
IPC->>FS: move merged_file_path → downloads_dir/filename
FS-->>IPC: moved_path
IPC->>IPC: handle_register_local_file(source_url, moved_path, filename, size)
IPC->>FS: persist completed download record
FS-->>IPC: DownloadId
IPC-->>Client: DownloadId
else CDN URL available
Loader-->>IPC: CdnUrl(url)
IPC->>IPC: derive filename (title/HEAD)
IPC->>Loader: handle_start_download(CdnUrl, filename, hostname_override)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
documentation
mpiton
changed the title
Greptile SummaryThis PR introduces a DASH fallback path for YouTube 1080p+ downloads: when The previous P1 concerns raised in earlier review rounds have been addressed: the Confidence Score: 5/5Safe to merge — all P0/P1 concerns from prior review rounds are resolved; no new issues found. The two P1 issues from previous rounds are fixed: the spurious active_count decrement (now gated by should_decrement) and the DownloadCompletedPersisted phantom variant (absent). Path-traversal protection is applied at two independent layers. All new code paths have unit tests. No remaining finding rises above P2. No files require special attention.
|
| Filename | Overview |
|---|---|
| src-tauri/src/adapters/driving/tauri_ipc.rs | Major refactor of download_media_start: adds StreamResolution enum, full DASH fallback branch with path-traversal double-check, sanitize_filename/sanitize_extension/unique_destination helpers, and extract_hostname_from_url. All helpers are well-tested. |
| src-tauri/src/application/commands/register_local_file.rs | New command handler that registers a yt-dlp-produced file as Completed. Validates file exists on disk, trusts on-disk size, drops DownloadStarted intentionally, with tests for edge cases. |
| src-tauri/src/application/services/queue_manager.rs | Fixes spurious active_count decrement via should_decrement gate. None-not-found case correctly skips decrement to avoid double-free on late completion racing a cancel. |
| src-tauri/src/adapters/driven/plugin/extism_loader.rs | Adds resolve_wasm_plugin helper, download_to_file with path-traversal canonicalization, and is_adaptive_stream_error matching the specific substring. Fragile string coupling documented. |
| src-tauri/src/application/commands/start_download.rs | Adds filename override (with path-traversal guard) and source_hostname_override, promotes next_download_id to pub(super). |
Sequence Diagram
sequenceDiagram
participant FE as Frontend
participant IPC as tauri_ipc
participant PL as ExtismPluginLoader
participant BUS as CommandBus
participant QM as QueueManager
FE->>IPC: download_media_start(url, "1080p", "mp4", title)
IPC->>IPC: sanitize_extension + extract_hostname
IPC->>PL: resolve_stream_url
PL-->>IPC: Err(AdaptiveStreamOnly)
IPC->>PL: download_to_file → yt-dlp DASH+ffmpeg
PL-->>IPC: DownloadedFileInfo{path, size}
IPC->>IPC: path-traversal check + unique_destination
IPC->>IPC: rename/copy to ~/Downloads
IPC->>BUS: handle_register_local_file
BUS->>BUS: save(Completed)
BUS->>QM: DownloadCreated → on_slot_freed
BUS->>QM: DownloadCompleted → no-decrement → on_slot_freed
IPC-->>FE: Ok(download_id)
Reviews (8): Last reviewed commit: "chore(deps): bump rustls-webpki 0.103.10..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry/registry.toml`:
- Around line 13-19: Update the manifest's declared compatibility: change
min_vortex_version in registry.toml (next to the version = "1.2.3" entry) from
"0.1.0" to the new minimum Vortex release that includes AdaptiveStreamOnly and
download_to_file support (e.g. "0.2.0"), so the registry advertises that this
plugin requires the newer host behavior; keep other fields (version,
checksum_sha256, checksum_sha256_toml, official) unchanged.
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 875-887: extract_hostname_from_url currently fails on URLs
containing userinfo (e.g., "user:pass@host") because it splits host_and_port by
':' and returns the "user" part; update extract_hostname_from_url to detect and
remove any userinfo by taking the substring after the last '@' in host_and_port
(if present) before splitting off the port, then proceed to return an
empty-check/final host String as before. Ensure the function still strips
schemes ("https://","http://","ftp://") and handles absent parts robustly
(returning None on failure).
- Around line 762-765: The fallback to "." when dirs::download_dir() returns
None is unsafe; change the logic in the download/save path to use a more
predictable fallback or surface an error: attempt
dirs::download_dir().or_else(|| dirs::home_dir()).or_else(||
dirs::document_dir()) and only if all are None return a handled error (propagate
Err or map to a Result) instead of writing to the CWD; update the code that
builds dest_dir and dest_path (look for dest_dir, dest_path, and filename in the
download/save routine) to use this new fallback order and ensure callers handle
the error return.
In `@src-tauri/src/application/commands/register_local_file.rs`:
- Around line 16-27: Validate the destination file on disk before persisting the
download as Completed: after creating Download::new(id, url, cmd.filename, dest)
and applying with_source_hostname, use std::fs::metadata (or tokio equivalent)
on cmd.destination_path to ensure the file exists and is a regular file, then
overwrite or set download.set_file_size(metadata.len() as u64) (instead of
trusting cmd.file_size) and only mark the Download as Completed after this
successful stat; if the file is missing or metadata fails, return an error or
set an appropriate non-completed status so we don't persist a Completed download
that points to a missing/stale file.
In `@src-tauri/src/application/commands/start_download.rs`:
- Around line 28-39: Reject any path-bearing or absolute overrides in
cmd.filename before using it as file_name: ensure the supplied string is
non-empty and is a single file component (no directory separators, no parent
components like "..", and not an absolute path). Replace the current let
file_name = if let Some(name) = cmd.filename.filter(|s| !s.is_empty()) { ... }
else { ... } logic with a validation step that checks
Path::new(name).components() contains exactly one Component::Normal (or compare
Path::new(name).file_name() == Some(os_str) and ensure not
starts_with("/")/windows drive) and only then accept name; otherwise fall back
to probing via head/extract_filename or filename_from_url. Use the validated
file_name when later calling dest_dir.join(&file_name).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 113733e2-ecbf-4995-9045-79b167b6a3b8
📒 Files selected for processing (10)
CHANGELOG.mdregistry/registry.tomlsrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/register_local_file.rssrc-tauri/src/application/commands/start_download.rssrc-tauri/src/domain/error.rssrc-tauri/src/domain/model/download.rssrc-tauri/src/domain/ports/driven/plugin_loader.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/application/commands/register_local_file.rs">
<violation number="1" location="src-tauri/src/application/commands/register_local_file.rs:18">
P1: Use `to_str()` with explicit error handling instead of `to_string_lossy()` to prevent silent path corruption. If the destination path contains non-UTF-8 characters, this will store a different path than where the file actually exists, causing file access failures.</violation>
</file>
<file name="src-tauri/src/application/commands/start_download.rs">
<violation number="1" location="src-tauri/src/application/commands/start_download.rs:31">
P0: Path traversal vulnerability: `cmd.filename` is used without sanitizing path separators, traversal sequences (..), or absolute paths. This allows writing files outside the intended directory via `PathBuf::join()`.</violation>
</file>
<file name="src-tauri/src/adapters/driving/tauri_ipc.rs">
<violation number="1" location="src-tauri/src/adapters/driving/tauri_ipc.rs:765">
P2: Check if destination file exists before overwriting to prevent silent data loss. Consider returning an error, adding a unique suffix, or prompting the user.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Path safety (P0):
- start_download: reject filename overrides that are absolute or contain path
components; `dest_dir.join(&file_name)` could previously escape the download
directory via `../` or an absolute path.
Data integrity (Major):
- register_local_file: stat destination_path before marking Completed — a
missing/stale file would otherwise persist a Completed entry pointing
nowhere. Use the on-disk size rather than the caller-reported one (files
can be post-processed between the reported size and registration).
- register_local_file: replace `to_string_lossy()` with `to_str()` so
non-UTF-8 paths fail loudly instead of silently storing a corrupted path
that cannot resolve to the real file.
Robustness (Minor):
- tauri_ipc: fall back to `home_dir` rather than `"."` when
`dirs::download_dir()` returns None; surface an error if neither is
available. Prevents 1GB merged videos from being dropped in the binary
directory.
- tauri_ipc::extract_hostname_from_url: strip `user:pass@` userinfo prefix
using `rsplit('@')` (handles passwords that contain '@' too).
- extism_loader::is_adaptive_stream_error: match the more specific
"adaptive stream (HLS/DASH)" substring and document the fragile
string-match coupling with vortex-mod-youtube ≥ 1.2.0.
Registry compatibility (Major):
- Bump Vortex 0.1.0 → 0.2.0 (minor bump per pre-1.0 semver — adds
AdaptiveStreamOnly + download_to_file).
- Bump `min_vortex_version` for vortex-mod-youtube 1.2.3 from 0.1.0 to
0.2.0 so the registry correctly advertises that this plugin requires
the new host behaviour.
Tests: 559 pass. Added 2 new register_local_file tests (missing file,
directory path) and 1 extract_hostname_from_url test covering userinfo.
Follow-up to the PR review (cubic P2, tauri_ipc.rs:765): when the merged yt-dlp file is moved to `~/Downloads/<title>.mp4` and a file with the same name already exists, append " (1)", " (2)", … to the name up to 9999 — matching the browser-download convention instead of silently overwriting a previous download. `unique_destination` returns both the final path and filename so the downstream RegisterLocalFileCommand stores the actually-used name. Tests: 4 cases (no collision, single collision, increment until free, extensionless name).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/adapters/driving/tauri_ipc.rs (1)
862-869: Consider sanitizing ASCII control characters.The function handles null bytes but not other control characters (ASCII 1-31), which are invalid in Windows filenames. While uncommon in video titles, they could appear in malformed metadata.
♻️ Suggested improvement
fn sanitize_filename(name: &str) -> String { let sanitized: String = name .chars() .map(|c| match c { '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' | '\0' => '_', + c if c.is_control() => '_', c => c, }) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driving/tauri_ipc.rs` around lines 862 - 869, The sanitize_filename function currently only replaces null bytes and a set of specific characters; update it to also treat other ASCII control characters (0x01–0x1F) as invalid by checking char::is_control() (or testing c <= '\u{1F}') and mapping those to '_' as well so filenames are safe on Windows and other platforms; modify the mapping in sanitize_filename to replace control characters in addition to the existing forbidden set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 773-786: The current rename/copy logic can silently overwrite an
existing file at dest_path; update the flow to detect existing destinations and
compute a non-conflicting final path before attempting file operations:
implement a small helper around dest_dir, filename (and file_stem/extension)
that loops while candidate.exists() and appends a numeric suffix like " (1)", "
(2)" to produce final_dest, then use final_dest in
std::fs::rename/std::fs::copy/remove_file and in tracing logs instead of
dest_path or filename; ensure the same helpers/variables are used by the
atomic-move fallback and preserve existing error/reporting behavior.
---
Nitpick comments:
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 862-869: The sanitize_filename function currently only replaces
null bytes and a set of specific characters; update it to also treat other ASCII
control characters (0x01–0x1F) as invalid by checking char::is_control() (or
testing c <= '\u{1F}') and mapping those to '_' as well so filenames are safe on
Windows and other platforms; modify the mapping in sanitize_filename to replace
control characters in addition to the existing forbidden set.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da1a03bf-7314-463c-b40d-31755761811c
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
package.jsonregistry/registry.tomlsrc-tauri/Cargo.tomlsrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/register_local_file.rssrc-tauri/src/application/commands/start_download.rssrc-tauri/tauri.conf.json
✅ Files skipped from review due to trivial changes (4)
- package.json
- src-tauri/tauri.conf.json
- src-tauri/Cargo.toml
- registry/registry.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- src-tauri/src/application/commands/register_local_file.rs
- src-tauri/src/adapters/driven/plugin/extism_loader.rs
- src-tauri/src/application/commands/start_download.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 884-893: The doc comments currently above unique_destination
belong to extract_hostname_from_url; move the comment block describing hostname
extraction to immediately above the extract_hostname_from_url function and
replace or update the comment above unique_destination to describe its behavior
(ensuring mention of probing "filename (1)", preserving extensions, and
returning (path, filename)). Update any function-level summaries so each
function (extract_hostname_from_url and unique_destination) has the correct,
specific documentation attached.
- Around line 749-753: The code constructs filenames with format_clone (via the
filename variable built from title_clone) and later uses them in LocalFile
paths/unique_destination/rename/copy, allowing crafted format values to escape
dest_dir; fix by validating/sanitizing format_clone before interpolation—ensure
format_clone is a single safe extension (e.g., match against an allowlist of
extensions or strip directory separators and leading dots, reject or normalize
values containing path separators like '/' or '..' or OS-specific separators),
then use the sanitized extension when building filename and when creating
LocalFile/unique_destination/rename/copy to prevent path traversal; update the
logic around filename creation (where title_clone/...map(|t| format!("{}.{}",
sanitize_filename(t), format_clone))) and the LocalFile
construction/unique_destination usage to use the validated/sanitized format.
- Around line 912-927: The loop that tries suffixes currently falls back to
returning the original path (returning (base, filename.to_string())) after 9,999
collisions, which silently reintroduces overwriting; change this to signal
failure instead of returning the original path — e.g., make the function return
a Result<(PathBuf, String), std::io::Error> (or an Option) and, when the suffix
loop in the block that references candidate_name, ext, stem, dir, base and
filename is exhausted, return
Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "too many name
collisions")) (or None) so callers must handle the collision error rather than
silently overwriting; update all callers of this function to handle the new
error/None case.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 991a3e6d-95a2-4284-b64d-f4fad1cc8513
📒 Files selected for processing (1)
src-tauri/src/adapters/driving/tauri_ipc.rs
- sanitize_extension: validate `format` at the IPC boundary of
download_media_start. The raw `format` parameter was splicing into
`format!("{title}.{format}")` which later reached
`dest_dir.join(&filename)` — a crafted `"../etc/passwd"` would escape
the download directory. Now rejects anything non-alphanumeric and
normalises to lowercase (accepts optional leading dot).
- unique_destination: return Result instead of silently overwriting
after 9999 collisions. The fallback-to-overwrite branch defeated the
purpose of the helper; surface an error and let the caller handle it.
Updated the call site in download_media_start to propagate with `?`.
- Fix misplaced doc comment: the old `extract_hostname_from_url`
rustdoc block ended up above `unique_destination` after insertion.
Moved back to the right function.
Tests: 4 new sanitize_extension cases (common exts, leading dot +
case, empty, path-traversal attacks). unique_destination tests updated
for the new Result signature. 567/567 pass.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 772-783: The code computes dest_dir from dirs::download_dir or
dirs::home_dir but doesn’t ensure that directory actually exists before calling
unique_destination; update the flow in the block that obtains dest_dir so that
after deriving dest_dir you check for its existence and create it if missing
(e.g. using std::fs::create_dir_all or equivalent) before calling
unique_destination(&dest_dir, &filename) and propagate any errors with the same
error formatting; keep references to dest_dir and unique_destination when making
the change so the directory creation occurs prior to selecting the unique
destination.
- Around line 743-753: The code currently trusts
plugin_loader.download_to_file(...) and uses file_info.path directly; validate
that the returned path is actually inside the temp_dir before any move/copy.
After calling plugin_loader.download_to_file (the call that returns file_info),
canonicalize both temp_dir and file_info.path (or otherwise resolve symlinks)
and check that file_info.path.starts_with(canonical_temp_dir); if the check
fails, return an error (e.g., map_err with a clear message) and do not proceed
to move/copy. Apply this validation at the tauri_ipc boundary where
download_to_file is invoked to enforce containment as a defense-in-depth
measure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 996203dc-199d-4c5c-9a76-c819fb4e2adf
📒 Files selected for processing (1)
src-tauri/src/adapters/driving/tauri_ipc.rs
- queue_manager::handle_download_completed: gate decrement_and_schedule on whether the download was in an active state before completion. RegisterLocalFileCommand publishes DownloadCompleted with the download already marked Completed — without this gate the scheduler thought a slot was freed that never existed, letting on_slot_freed start one extra download above max_concurrent for every local-file registration. The `None` (race-delete) branch still decrements to avoid leaking slots. - tauri_ipc: re-check plugin download_to_file output path containment at the IPC boundary. The PluginLoader trait does not require implementations to enforce output_dir containment (ExtismPluginLoader does, but a future alt loader might not). Canonicalise both temp_dir and the produced path and reject if the latter escapes the former. - tauri_ipc: create_dir_all(dest_dir) before selecting unique_destination. user-dirs' Downloads folder can be configured but non-existent (fresh account, CI) and rename/copy would fail opaquely later in the flow. Tests: new test_handle_completed_skips_decrement_when_not_active covers the queue-manager gate. 568/568 pass.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/application/services/queue_manager.rs`:
- Around line 196-202: handle_download_completed's None branch currently returns
true which causes the scheduler slot to be freed even when the repo row was
already removed (e.g., a prior DownloadCancelled already decremented), allowing
double-decrement races; change the None branch in handle_download_completed (the
arm logging "handle_download_completed: download not found in repo") to return
false (i.e., do not free a slot) so only existing repo rows lead to slot
release, and apply the same fix to the analogous None branch at the other
occurrence (lines 205-207) to prevent double-decrementing when a late
DownloadCompleted arrives after DownloadCancelled.
- Around line 744-756: The test uses make_download(...,
DownloadState::Completed) but the helper doesn't actually produce a Completed
record; update the fixture so the repo contains a true completed download before
calling qm.handle_download_completed: either adjust make_download to set the
state to Completed (or call the helper method that transitions a Download to
Completed) and ensure MockDownloadRepo holds that completed item; verify the
test uses the same DownloadId(1) and existing repo/engine/bus fixtures
(make_download, MockDownloadRepo, make_manager, qm.handle_download_completed) so
the handler exercises the intended pre-completed local-file branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0c7b833-9348-426c-b66e-3ecab00cbfa5
📒 Files selected for processing (2)
src-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/services/queue_manager.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/adapters/driving/tauri_ipc.rs
- handle_download_completed: on `None` (row not found), skip the decrement instead of assuming the slot was occupied. The only realistic way the row disappears is via remove_download, which already publishes DownloadCancelled (→ decrement). A late DownloadCompleted racing a cancel would otherwise double-free the same slot. - Fix test_handle_completed_skips_decrement_when_not_active: the previous setup called make_download(..., DownloadState::Completed) but the helper's match arm for Completed falls through without applying the Queued→Downloading→Completed transitions, so the fixture was actually Queued. The regression was being tested against the wrong state. Build the download through the real state machine and assert it really ends in Completed. - Update test_handle_download_completed_with_missing_download to reflect the new no-decrement-on-None semantics. The greptile P0 alarm about a missing DownloadCompletedPersisted variant is a false positive — the variant exists in domain/event.rs (and is wired through tauri_bridge + download_log_bridge). The branch compiles and 568/568 tests pass.
CI's `cargo fmt --check` was failing on these 5 files — partly from pre-existing unformatted test code and partly from my recent additions that rustfmt wants broken across multiple lines (long assertion macros, multi-arg constructor calls, compact trait impls in test mocks). Ran rustfmt on each file individually to avoid disturbing two unrelated files (tauri_bridge.rs, plugin/registry.rs) that the user still has as uncommitted WIP. Those two will need a follow-up fmt pass from the user once they commit their changes. 568/568 tests still pass.
My earlier refactor of handle_download_completed (commit 67ca699) picked up an extra `publish(DownloadCompletedPersisted)` call and a matching test from the working tree — both still part of the user's uncommitted WIP that introduces the new DomainEvent variant. The variant itself was not committed, so CI's clippy step failed with error[E0599]: no variant named `DownloadCompletedPersisted` found for enum `domain::event::DomainEvent` Drop both the stray publish and the dependent test. When the WIP lands, the publish and test can be reintroduced together with the variant.
RUSTSEC-2026-0097 flags rand 0.9.2 as unsound with a custom logger using `rand::rng()`. Advisory was published 2026-04-09 and started failing this branch's CI once the advisory DB picked it up. `cargo update -p rand@0.9.2` cleanly upgrades the transitive dependency (pulled in by tauri-plugin-notification and quinn-proto via reqwest) to the patched 0.9.4. Note: main's CI is also red on `cargo audit` for ~13 other pre-existing advisories (rsa, rustls-webpki, wasmtime). Those are outside the scope of this PR — this commit only resolves the one new advisory I can trace to my own dependency changes.
Clears RUSTSEC-2026-0098 and RUSTSEC-2026-0099 (both name-constraint handling bugs in rustls-webpki). `cargo update -p rustls-webpki` finds a compatible patch release without changing any other pin. The remaining `cargo audit` failures are in deps that this PR cannot address: - rand 0.7.3 / 0.8.5 pulled in by tauri-utils → kuchikiki → phf - wasmtime 2026-0085…0096 pulled in by extism - rsa RUSTSEC-2023-0071 transitive - GTK3 bindings (atk, gdk, gtk, …) flagged "no longer maintained" These are pre-existing on main (main's CI has been red on audit since 2026-04-15) and need a separate maintenance PR to either bump Tauri itself or to document the accepted risk in `.cargo/audit.toml`.
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.
Summary
Ajoute le chemin de téléchargement
download_to_filequi délègue la fusion DASH (video + audio via ffmpeg) àyt-dlppour les qualités YouTube ≥720p. Jusqu'ici, Vortex téléchargeait silencieusement en 360p parce que les flux pré-mergés HTTPS de YouTube sont plafonnés à 480p.Changes
Domain (
src-tauri/src/domain/)DomainError::AdaptiveStreamOnly— signale qu'une URL n'est disponible qu'en DASH/HLS et doit passer pardownload_to_fileDownloadedFileInfo { path, size }sur le portPluginLoaderPluginLoader::download_to_file(url, quality, format, output_dir, audio_only) -> DownloadedFileInfoDownload::set_file_size(u64)pour enregistrer la taille finale d'un fichier déjà téléchargéApplication (
src-tauri/src/application/)RegisterLocalFileCommand+ handler — enregistre un fichier local déjà téléchargé commeCompleted(transitionQueued → Downloading → Completedatomique)next_download_id()partagé (pub(super)) entrestart_downloadetregister_local_filepour éviter les collisions d'IDDriven adapters (
src-tauri/src/adapters/driven/plugin/)download_to_fileavec protection path traversal (canonicalisation +starts_with(output_dir))resolve_wasm_plugin(url)partagé entreresolve_stream_urletdownload_to_fileis_adaptive_stream_error(msg)mappe les erreurs du plugin WASM versDomainError::AdaptiveStreamOnlyDriving adapter (
src-tauri/src/adapters/driving/tauri_ipc.rs)download_media_startrefactorisé autour d'une enum localeStreamResolution { CdnUrl(String), LocalFile { path, size, filename } }Ok(cdn_url)→ téléchargement segmenté classique viaStartDownloadCommandErr(AdaptiveStreamOnly)→plugin_loader.download_to_file→ copie dans le download dir →RegisterLocalFileCommandErr(NotFound)avecis_known_media_platform→ erreur claire "install the plugin" au lieu de feeder l'URL HTML à l'enginetemp_dir.to_str()avec propagation d'erreur explicitePlugin registry
vortex-mod-youtube: 1.1.x → 1.2.3 (ligne de release cumulative)download_to_fileAdaptiveStreamOnlypour height ≥720 (évite le fallback silencieux 360p)Related Issues
Fixes #59 (YouTube downloads limited to 360p despite 1080p selection).
Type of Change
Checklist
cargo fmtandoxlintpasscargo clippy -- -D warningsis cleancargo test+npm test)CHANGELOG.mdunder[Unreleased]Testing
E2E manuel (via
tauri-pilot ipc, app Tauri en dev) :Completed✅Completed✅ (précédemment 403 avant v1.2.3)Vérification pipeline : les fichiers sortants sont av1 (DASH) + opus, cohérent avec le chemin
download_to_file(yt-dlp merge DASH via ffmpeg). Un fichier issu du chemin legacy aurait étéavc1 + aac(itag 18, ≤360p pré-mergé).Summary by CodeRabbit
Bug Fixes
New Features
Chores