Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughAdds Deno provisioning/upgrade and threads the resolved Deno binary into yt-dlp invocations via a new YtDlpDownload.deno_bin field; main captures and passes the returned deno path. Also updates yt-dlp CLI args/progress, and tweaks logging in the download loop, Dropbox handling, and Telegram failed-downloads header. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Main
participant prepare_yt_dlp as prepare_yt_dlp()
participant DenoInstaller as Deno installer (curl/sh)
participant DenoBin as deno binary (~/.deno/bin/deno)
participant YtDlpDownload as YtDlpDownload
participant yt_dlp as yt-dlp process
Main->>prepare_yt_dlp: call with yt_dlp_executable_path
prepare_yt_dlp->>DenoInstaller: install if missing / run upgrade
DenoInstaller-->>DenoBin: produce deno binary path
prepare_yt_dlp-->>Main: return deno_bin PathBuf
Main->>YtDlpDownload: construct with deno_bin
YtDlpDownload->>yt_dlp: spawn yt-dlp with --js-runtimes deno:<deno_bin> and args
yt_dlp-->>YtDlpDownload: progress / exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR extends the YouTube download manager’s startup preparation to also install/update Deno in addition to updating yt-dlp.
Changes:
- Added a new
update_deno()function that runscargo install deno --version 2.7.10 --locked. - Updated
prepare_yt_dlp()to runupdate_yt_dlp(...).await?and thenupdate_deno().await.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| update_yt_dlp(yt_dlp_executable_path).await?; | ||
| update_deno().await |
There was a problem hiding this comment.
prepare_yt_dlp now unconditionally runs cargo install deno ... on startup. This introduces a hard runtime dependency on a Rust toolchain + network access and will fail in typical deployment environments where cargo is not available, preventing the app from starting even when yt-dlp is present. If Deno is required, consider checking for an existing deno binary (and optionally its version) and returning a clear instruction to install it out-of-band, or gate this behind a config/feature flag rather than installing during normal execution.
Coverage Report for CI Build 24276374754Coverage decreased (-0.5%) to 73.513%Details
Uncovered Changes
Coverage Regressions3 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@download_manager/src/yt_dlp.rs`:
- Around line 55-137: The Deno install/update flow currently calls "bash"
(install_deno) and hardcodes ~/.deno/bin/deno (update_deno), which fails on
Windows and ignores DENO_INSTALL/PATH; change
install_deno/upgrade_deno/update_deno to (1) detect platform and binary name
("deno" vs "deno.exe"), (2) first look for an existing binary on PATH
(std::env::var_os("PATH") + check each dir for the binary) and then check
DENO_INSTALL env var to construct the expected path (join DENO_INSTALL with
"bin/deno" or "bin\\deno.exe"), (3) avoid invoking bash on Windows—use
platform-appropriate installer logic: on Unix you can keep the script, on
Windows use PowerShell or download the prebuilt release archive and extract to a
target install dir, and (4) update upgrade_deno/install_deno to accept or return
the resolved PathBuf so update_deno no longer assumes "~/.deno/bin/deno". Ensure
all existence checks use the detected binary name and prefer PATH/DENO_INSTALL
before attempting installation.
- Around line 55-137: The startup currently runs install_deno/upgrade_deno (and
prepare_yt_dlp) which downloads and executes unpinned remote scripts, making
process start-up network-dependent and insecure; remove or disable automatic
runtime installation/upgrade from startup (stop calling install_deno,
upgrade_deno, and update_deno from prepare_yt_dlp/main) and instead fail fast if
the required deno binary is missing, documenting that a pinned, preinstalled
deno must be provided (or a DENO_BIN env var/path should be set), or implement a
secure, explicit update command that fetches a versioned artefact and verifies
its checksum/signature before replacing the binary; also avoid piping remote
scripts into sh in install_deno and replace with an explicit, auditable
installation workflow if you must keep an installer function.
🪄 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: ea1c279c-c001-4e08-a268-466ae8a19627
📒 Files selected for processing (3)
download_manager/src/main.rsdownload_manager/src/telegram_bot.rsdownload_manager/src/yt_dlp.rs
| async fn install_deno() -> Result<(), Box<dyn std::error::Error>> { | ||
| let mut cmd = Command::new("bash"); | ||
| cmd.arg("-c"); | ||
| // https://docs.deno.com/runtime/getting_started/installation/ | ||
| cmd.arg("curl -fsSL https://deno.land/install.sh | sh"); | ||
| cmd.stdout(Stdio::piped()); | ||
| let mut child = cmd.spawn()?; | ||
| let stdout = child | ||
| .stdout | ||
| .take() | ||
| .expect("child did not have a handle to stdout"); | ||
| let mut reader = BufReader::new(stdout).lines(); | ||
| let (status_result, read_result): (std::io::Result<ExitStatus>, std::io::Result<()>) = | ||
| tokio::join!(child.wait(), async move { | ||
| while let Some(line) = reader.next_line().await? { | ||
| info!("deno: {}", line); | ||
| } | ||
| Ok(()) | ||
| }); | ||
| let status = status_result?; | ||
| info!("Child status was: {}", status); | ||
| if status.success() { | ||
| info!("deno installed successfully"); | ||
| } else { | ||
| let message = format!("deno installation exited with {status}"); | ||
| error!("{message}"); | ||
| return Err(Box::from(message)); | ||
| } | ||
| read_result?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn upgrade_deno(deno_bin: &std::path::Path) -> Result<(), Box<dyn std::error::Error>> { | ||
| let mut cmd = Command::new(deno_bin); | ||
| cmd.arg("upgrade"); | ||
| cmd.stdout(Stdio::piped()); | ||
| let mut child = cmd.spawn()?; | ||
| let stdout = child | ||
| .stdout | ||
| .take() | ||
| .expect("child did not have a handle to stdout"); | ||
| let mut reader = BufReader::new(stdout).lines(); | ||
| let (status_result, read_result): (std::io::Result<ExitStatus>, std::io::Result<()>) = | ||
| tokio::join!(child.wait(), async move { | ||
| while let Some(line) = reader.next_line().await? { | ||
| info!("deno: {}", line); | ||
| } | ||
| Ok(()) | ||
| }); | ||
| let status = status_result?; | ||
| info!("Child status was: {}", status); | ||
| if status.success() { | ||
| info!("deno upgraded successfully"); | ||
| } else { | ||
| let message = format!("deno upgrade exited with {status}"); | ||
| error!("{message}"); | ||
| return Err(Box::from(message)); | ||
| } | ||
| read_result?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn update_deno() -> Result<PathBuf, Box<dyn std::error::Error>> { | ||
| let user_home = match std::env::home_dir() { | ||
| Some(path) => path, | ||
| None => { | ||
| let message = "Failed to determine user home directory".to_string(); | ||
| return Err(Box::from(message)); | ||
| } | ||
| }; | ||
| let deno_bin = user_home.join(".deno/bin/deno"); | ||
| if !deno_bin.try_exists()? { | ||
| install_deno().await?; | ||
| if !deno_bin.try_exists()? { | ||
| let message = format!( | ||
| "deno was not found at {} after installation", | ||
| deno_bin.display() | ||
| ); | ||
| return Err(Box::from(message)); | ||
| } | ||
| } | ||
| upgrade_deno(&deno_bin).await?; | ||
| Ok(deno_bin) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file structure first
git ls-files | grep -E "(yt_dlp|main)" | head -20Repository: TyRoXx/NonlocalityOS
Length of output: 327
🏁 Script executed:
# Get the actual code from yt_dlp.rs around the mentioned lines
head -150 download_manager/src/yt_dlp.rs | tail -100Repository: TyRoXx/NonlocalityOS
Length of output: 3360
🏁 Script executed:
# Also check main.rs around lines 932-935 for Windows handling
sed -n '925,945p' download_manager/src/main.rsRepository: TyRoXx/NonlocalityOS
Length of output: 721
🏁 Script executed:
# Search for DENO_INSTALL usage in the codebase
rg "DENO_INSTALL" download_manager/Repository: TyRoXx/NonlocalityOS
Length of output: 46
🏁 Script executed:
# Search for deno_bin path construction
rg "deno.bin\|\.deno.*bin" download_manager/ -A 2 -B 2Repository: TyRoXx/NonlocalityOS
Length of output: 46
🏁 Script executed:
# Check if home_dir is used elsewhere and verify its deprecation status
rg "home_dir" download_manager/Repository: TyRoXx/NonlocalityOS
Length of output: 143
🏁 Script executed:
# Check the full update_deno function to understand the path construction
sed -n '118,145p' download_manager/src/yt_dlp.rsRepository: TyRoXx/NonlocalityOS
Length of output: 971
🏁 Script executed:
# Check if there's any PATH-based deno resolution or which command
rg "which\|PATH\|find.*deno" download_manager/Repository: TyRoXx/NonlocalityOS
Length of output: 46
🏁 Script executed:
# Check what platform-specific code exists for other tools
rg "cfg\(target_os" download_manager/ -A 3 | head -40Repository: TyRoXx/NonlocalityOS
Length of output: 299
🏁 Script executed:
# Look at the complete prepare_yt_dlp function to understand integration
rg -A 30 "fn prepare_yt_dlp" download_manager/src/yt_dlp.rsRepository: TyRoXx/NonlocalityOS
Length of output: 1240
Deno binary discovery must support Windows and custom installation locations.
The code at line 56 invokes bash explicitly, which is unavailable by default on Windows. At lines 118–125, the code hard-codes the path to ~/.deno/bin/deno, ignoring the DENO_INSTALL environment variable and any PATH-based installations. Unlike the platform-aware handling for yt-dlp in main.rs (platform-specific executable naming and cfg guards), there is no equivalent support for Deno, breaking Windows deployments entirely.
Instead, resolve the Deno binary by checking DENO_INSTALL, searching PATH, detecting the platform-specific executable name (deno vs deno.exe), and using a platform-independent installer or a pre-built binary download approach that does not depend on shell availability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@download_manager/src/yt_dlp.rs` around lines 55 - 137, The Deno
install/update flow currently calls "bash" (install_deno) and hardcodes
~/.deno/bin/deno (update_deno), which fails on Windows and ignores
DENO_INSTALL/PATH; change install_deno/upgrade_deno/update_deno to (1) detect
platform and binary name ("deno" vs "deno.exe"), (2) first look for an existing
binary on PATH (std::env::var_os("PATH") + check each dir for the binary) and
then check DENO_INSTALL env var to construct the expected path (join
DENO_INSTALL with "bin/deno" or "bin\\deno.exe"), (3) avoid invoking bash on
Windows—use platform-appropriate installer logic: on Unix you can keep the
script, on Windows use PowerShell or download the prebuilt release archive and
extract to a target install dir, and (4) update upgrade_deno/install_deno to
accept or return the resolved PathBuf so update_deno no longer assumes
"~/.deno/bin/deno". Ensure all existence checks use the detected binary name and
prefer PATH/DENO_INSTALL before attempting installation.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the yt_dlp.rs file and check its size
find . -name "yt_dlp.rs" -type fRepository: TyRoXx/NonlocalityOS
Length of output: 96
🏁 Script executed:
# Get the full content of the file to verify the claims
cat -n download_manager/src/yt_dlp.rs | head -350Repository: TyRoXx/NonlocalityOS
Length of output: 14160
🏁 Script executed:
# Search for the prepare_yt_dlp function and where update_deno is called
rg -n "prepare_yt_dlp|update_deno" download_manager/src/yt_dlp.rs -A 5 -B 2Repository: TyRoXx/NonlocalityOS
Length of output: 973
🏁 Script executed:
# Search for calls to prepare_yt_dlp across the codebase
rg -n "prepare_yt_dlp" --type rust -B 3 -A 3Repository: TyRoXx/NonlocalityOS
Length of output: 1071
🏁 Script executed:
# Also search for main.rs or similar entry points
find . -name "main.rs" -o -name "lib.rs" | head -5Repository: TyRoXx/NonlocalityOS
Length of output: 213
🏁 Script executed:
# Get context around line 937 in main.rs to see if this is during startup
cat -n download_manager/src/main.rs | sed -n '920,960p'Repository: TyRoXx/NonlocalityOS
Length of output: 1629
🏁 Script executed:
# Check what function this is in - look for the function definition
cat -n download_manager/src/main.rs | sed -n '900,945p'Repository: TyRoXx/NonlocalityOS
Length of output: 1977
🏁 Script executed:
# Find the function definition that contains line 937
cat -n download_manager/src/main.rs | sed -n '850,900p'Repository: TyRoXx/NonlocalityOS
Length of output: 2422
🏁 Script executed:
# Search for the function definition - look earlier
cat -n download_manager/src/main.rs | sed -n '800,850p'Repository: TyRoXx/NonlocalityOS
Length of output: 2361
🏁 Script executed:
# Also search for 'async fn' and 'fn main' around this area
rg -n "^(async )?fn " download_manager/src/main.rs | tail -5Repository: TyRoXx/NonlocalityOS
Length of output: 285
Avoid installing and self-upgrading Deno during normal application start-up.
The code at line 59 pipes a downloaded script into a shell, and lines 331–333 execute prepare_yt_dlp() during application startup in main() (line 937). This makes process start-up depend on network availability and executes unpinned upstream code with user privileges before any download job can run. A transient outage or upstream regression can now take the whole service down. Prefer a pinned, preinstalled runtime, or fetch a versioned artefact and verify it before execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@download_manager/src/yt_dlp.rs` around lines 55 - 137, The startup currently
runs install_deno/upgrade_deno (and prepare_yt_dlp) which downloads and executes
unpinned remote scripts, making process start-up network-dependent and insecure;
remove or disable automatic runtime installation/upgrade from startup (stop
calling install_deno, upgrade_deno, and update_deno from prepare_yt_dlp/main)
and instead fail fast if the required deno binary is missing, documenting that a
pinned, preinstalled deno must be provided (or a DENO_BIN env var/path should be
set), or implement a secure, explicit update command that fetches a versioned
artefact and verifies its checksum/signature before replacing the binary; also
avoid piping remote scripts into sh in install_deno and replace with an
explicit, auditable installation workflow if you must keep an installer
function.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
download_manager/src/yt_dlp.rs (1)
55-137: Please extract Deno preparation into a testable helper.This path is currently uncovered, and it mixes path resolution, installation, upgrade and start-up orchestration into one branch. Pulling the pure bits into a small resolver/command-builder would make the missing-binary and failure cases cheap to cover.
Also applies to: 285-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@download_manager/src/yt_dlp.rs` around lines 55 - 137, The update logic in update_deno currently mixes path resolution, installation, upgrade and orchestration making it hard to test; extract pure, testable helpers: add a resolver function (e.g., resolve_deno_bin() -> Result<PathBuf, Error>) that only computes the deno_bin path and missing-binary conditions, and add command-builder helpers (e.g., build_install_command() and build_upgrade_command() that return Command or a serializable command-spec like Vec<String>) so install_deno and upgrade_deno only perform execution and logging; refactor update_deno to call resolve_deno_bin() and the builders, and write tests against the resolver and builders to cover missing-binary and failure scenarios without spawning processes (mock/execute in separate integration tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@download_manager/src/dropbox.rs`:
- Around line 236-239: The loop currently breaks when
list_folder_result.entries.is_empty(), which can prematurely stop processing
before pagination finishes; instead, in the loop that processes Dropbox pages
(using list_folder_result, has_more, cursor, from_directory), change the control
flow so an empty entries page does not break the loop—either skip processing
that page (continue) and then handle pagination by checking
list_folder_result.has_more and advancing the cursor, or move the empty-page
check after the has_more/cursor update so you only exit when has_more is false;
update the loop surrounding the Dropbox list_folder_result handling accordingly.
In `@download_manager/src/yt_dlp.rs`:
- Around line 126-136: The code always calls upgrade_deno(&deno_bin).await? even
after a fresh install, causing startup to fail if the upgrade step transiently
errors; change the flow so you only call upgrade_deno when deno_bin already
existed before install: check deno_bin.try_exists() first, if false call
install_deno().await? and re-check existence (as currently done) but only call
upgrade_deno(&deno_bin).await? when the initial existence check returned true
(i.e., the binary was present), avoiding the second network hop after a
successful install; relevant symbols: deno_bin, try_exists(), install_deno(),
upgrade_deno().
---
Nitpick comments:
In `@download_manager/src/yt_dlp.rs`:
- Around line 55-137: The update logic in update_deno currently mixes path
resolution, installation, upgrade and orchestration making it hard to test;
extract pure, testable helpers: add a resolver function (e.g.,
resolve_deno_bin() -> Result<PathBuf, Error>) that only computes the deno_bin
path and missing-binary conditions, and add command-builder helpers (e.g.,
build_install_command() and build_upgrade_command() that return Command or a
serializable command-spec like Vec<String>) so install_deno and upgrade_deno
only perform execution and logging; refactor update_deno to call
resolve_deno_bin() and the builders, and write tests against the resolver and
builders to cover missing-binary and failure scenarios without spawning
processes (mock/execute in separate integration tests).
🪄 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: c535a195-cf43-4774-8a8f-8b8bafec148f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomldownload_manager/src/dropbox.rsdownload_manager/src/main.rsdownload_manager/src/yt_dlp.rs
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- download_manager/src/main.rs
| if list_folder_result.entries.is_empty() { | ||
| info!("No entries found in {}, nothing to move", from_directory); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Avoid breaking before pagination completes.
Line 236–Line 239 now exits the loop before the has_more check, so an empty page can terminate processing even when more pages are still available.
Proposed fix
- if list_folder_result.entries.is_empty() {
- info!("No entries found in {}, nothing to move", from_directory);
- break;
- }
- info!(
- "Processing a chunk of {} directory entries in {}",
- list_folder_result.entries.len(),
- from_directory
- );
+ if list_folder_result.entries.is_empty() {
+ if !list_folder_result.has_more {
+ info!("No entries found in {}, nothing to move", from_directory);
+ break;
+ }
+ debug!(
+ "Received an empty page from Dropbox for {}, continuing pagination",
+ from_directory
+ );
+ } else {
+ info!(
+ "Processing a chunk of {} directory entries in {}",
+ list_folder_result.entries.len(),
+ from_directory
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@download_manager/src/dropbox.rs` around lines 236 - 239, The loop currently
breaks when list_folder_result.entries.is_empty(), which can prematurely stop
processing before pagination finishes; instead, in the loop that processes
Dropbox pages (using list_folder_result, has_more, cursor, from_directory),
change the control flow so an empty entries page does not break the loop—either
skip processing that page (continue) and then handle pagination by checking
list_folder_result.has_more and advancing the cursor, or move the empty-page
check after the has_more/cursor update so you only exit when has_more is false;
update the loop surrounding the Dropbox list_folder_result handling accordingly.
| if !deno_bin.try_exists()? { | ||
| install_deno().await?; | ||
| if !deno_bin.try_exists()? { | ||
| let message = format!( | ||
| "deno was not found at {} after installation", | ||
| deno_bin.display() | ||
| ); | ||
| return Err(Box::from(message)); | ||
| } | ||
| } | ||
| upgrade_deno(&deno_bin).await?; |
There was a problem hiding this comment.
Skip the second network hop after a fresh install.
When deno_bin is missing, this path installs Deno and then immediately runs deno upgrade anyway. A transient failure in that second step now aborts start-up even though the runtime was already installed successfully. Only upgrade when an existing binary was found.
🛠️ Proposed fix
async fn update_deno() -> Result<PathBuf, Box<dyn std::error::Error>> {
@@
- if !deno_bin.try_exists()? {
+ let newly_installed = if !deno_bin.try_exists()? {
install_deno().await?;
if !deno_bin.try_exists()? {
let message = format!(
"deno was not found at {} after installation",
deno_bin.display()
);
return Err(Box::from(message));
}
- }
- upgrade_deno(&deno_bin).await?;
+ true
+ } else {
+ false
+ };
+ if !newly_installed {
+ upgrade_deno(&deno_bin).await?;
+ }
Ok(deno_bin)
}📝 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 !deno_bin.try_exists()? { | |
| install_deno().await?; | |
| if !deno_bin.try_exists()? { | |
| let message = format!( | |
| "deno was not found at {} after installation", | |
| deno_bin.display() | |
| ); | |
| return Err(Box::from(message)); | |
| } | |
| } | |
| upgrade_deno(&deno_bin).await?; | |
| let newly_installed = if !deno_bin.try_exists()? { | |
| install_deno().await?; | |
| if !deno_bin.try_exists()? { | |
| let message = format!( | |
| "deno was not found at {} after installation", | |
| deno_bin.display() | |
| ); | |
| return Err(Box::from(message)); | |
| } | |
| true | |
| } else { | |
| false | |
| }; | |
| if !newly_installed { | |
| upgrade_deno(&deno_bin).await?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@download_manager/src/yt_dlp.rs` around lines 126 - 136, The code always calls
upgrade_deno(&deno_bin).await? even after a fresh install, causing startup to
fail if the upgrade step transiently errors; change the flow so you only call
upgrade_deno when deno_bin already existed before install: check
deno_bin.try_exists() first, if false call install_deno().await? and re-check
existence (as currently done) but only call upgrade_deno(&deno_bin).await? when
the initial existence check returned true (i.e., the binary was present),
avoiding the second network hop after a successful install; relevant symbols:
deno_bin, try_exists(), install_deno(), upgrade_deno().
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
download_manager/src/yt_dlp.rs (1)
125-149: Please add tests around the Deno resolution and argv branches.This is the riskiest part of the patch, but Coveralls shows 0% coverage for the changed lines in this file. Pull the path-resolution/install decision and the
--js-runtimesargument building into small helpers so they can be unit-tested without spawning external processes.Also applies to: 170-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@download_manager/src/yt_dlp.rs` around lines 125 - 149, The update_deno function’s path-resolution and install/upgrade decision logic (use of std::env::home_dir, construction of deno_bin, try_exists check, and calls to install_deno/upgrade_deno) and the code that builds the --js-runtimes argv should be extracted into small pure helper functions so they can be unit-tested without spawning processes; add e.g. a resolve_deno_path(home_dir: Option<PathBuf>) -> PathBuf (or similar) and a needs_install_or_upgrade(deno_bin: &Path) -> bool and a build_js_runtimes_args(config) -> Vec<String>, move the relevant logic out of update_deno to call those helpers, and write unit tests for those helpers covering both existing/missing Deno paths and different --js-runtimes branches (mock installation behavior by testing helpers independently of install_deno/upgrade_deno).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@download_manager/src/yt_dlp.rs`:
- Around line 197-199: The current invocation always injects "--js-runtimes
deno:..." which makes Deno a hard prerequisite; change the call site in
prepare_yt_dlp()/where you build the Command so you only push "--js-runtimes"
and the "deno:..." argument when the target URL/extractor actually requires a JS
runtime. Detect this by inspecting the video_url (or using yt-dlp to probe the
extractor) and only call cmd.arg("--js-runtimes") and cmd.arg(format!("deno:{}",
deno_bin.display())) when that check indicates a Deno-dependent extractor;
otherwise leave the command as the plain yt-dlp invocation so missing/broken
Deno won’t block unrelated downloads. Ensure the conditional logic is applied
both where the args are currently added (lines around cmd.arg("--js-runtimes")
and the duplicate location mentioned).
---
Nitpick comments:
In `@download_manager/src/yt_dlp.rs`:
- Around line 125-149: The update_deno function’s path-resolution and
install/upgrade decision logic (use of std::env::home_dir, construction of
deno_bin, try_exists check, and calls to install_deno/upgrade_deno) and the code
that builds the --js-runtimes argv should be extracted into small pure helper
functions so they can be unit-tested without spawning processes; add e.g. a
resolve_deno_path(home_dir: Option<PathBuf>) -> PathBuf (or similar) and a
needs_install_or_upgrade(deno_bin: &Path) -> bool and a
build_js_runtimes_args(config) -> Vec<String>, move the relevant logic out of
update_deno to call those helpers, and write unit tests for those helpers
covering both existing/missing Deno paths and different --js-runtimes branches
(mock installation behavior by testing helpers independently of
install_deno/upgrade_deno).
🪄 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: 9bcfe77b-5766-46e1-bff5-36034141440c
📒 Files selected for processing (1)
download_manager/src/yt_dlp.rs
| cmd.arg("--js-runtimes"); | ||
| cmd.arg(format!("deno:{}", deno_bin.display())); | ||
| cmd.arg(video_url); |
There was a problem hiding this comment.
Do not make Deno a hard prerequisite for every yt-dlp download.
The inline comment says Deno is needed for YouTube, but this now resolves Deno in prepare_yt_dlp() and injects --js-runtimes on every invocation. A missing or broken Deno install will now block unrelated yt-dlp downloads as well. Please resolve/pass the runtime only for extractors that actually need it, or fall back to the plain invocation for other URLs.
Also applies to: 344-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@download_manager/src/yt_dlp.rs` around lines 197 - 199, The current
invocation always injects "--js-runtimes deno:..." which makes Deno a hard
prerequisite; change the call site in prepare_yt_dlp()/where you build the
Command so you only push "--js-runtimes" and the "deno:..." argument when the
target URL/extractor actually requires a JS runtime. Detect this by inspecting
the video_url (or using yt-dlp to probe the extractor) and only call
cmd.arg("--js-runtimes") and cmd.arg(format!("deno:{}", deno_bin.display()))
when that check indicates a Deno-dependent extractor; otherwise leave the
command as the plain yt-dlp invocation so missing/broken Deno won’t block
unrelated downloads. Ensure the conditional logic is applied both where the args
are currently added (lines around cmd.arg("--js-runtimes") and the duplicate
location mentioned).
TyRoXx
force-pushed
the
download_youtube
branch
from
5c62e2f to
06daf91
Compare
Summary by CodeRabbit
New Features
Improvements
Chores