Conversation
|
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:
WalkthroughAdds content-hash-aware error handling for Dropbox moves: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as move_files / caller
participant Move as move_file
participant Relocate as Dropbox API (files/move_v2)
participant Meta as Dropbox API (get_metadata)
participant Delete as Dropbox API (delete)
participant Log as Logger
Caller->>Move: request move(source_path, dest_path)
Move->>Relocate: files/move_v2(relocation_args)
alt relocation succeeds
Relocate-->>Move: success
Move-->>Caller: return success
else relocation fails
Relocate-->>Move: error
Move->>Log: warn("relocation failed — invoking hash handler")
Move->>Meta: get_metadata(source_path)
Move->>Meta: get_metadata(dest_path)
Meta-->>Move: source_hash, dest_hash (or None)
Move->>Log: info("compare hashes")
alt hashes match
Move->>Delete: delete(source_path)
Delete-->>Move: delete result
Move-->>Caller: treated as moved / resolved
else hashes differ or missing
Move->>Log: error("hash mismatch or missing — move failed")
Move-->>Caller: return error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 enhances error handling for Dropbox file relocation operations by adding intelligent recovery logic when move operations fail. The changes implement a strategy to detect if a file already exists at the destination with identical content, and if so, safely delete the source file instead of treating it as a critical error.
Changes:
- Added
get_file_content_hashfunction to retrieve and validate file metadata - Added
handle_move_file_errorfunction to implement error recovery logic by comparing content hashes - Modified
move_fileto use&strparameters instead of ownedStringand call error handler on failures
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Pull Request Test Coverage Report for Build 22219787165Details
💛 - 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: 1
🧹 Nitpick comments (2)
download_manager/src/dropbox.rs (2)
197-198:warn!fires unconditionally before severity is known, producing noisy logs for the expected idempotent-move case.When
handle_move_file_errordetermines that the destination already holds the same content (hash match → line 153), it logs atinfo!. However, thewarn!on line 197 has already emitted, leaving a misleading warning in the log even for the entirely benign scenario. Consider deferring thewarn!/error!decision until afterhandle_move_file_errorreports its outcome, or at minimum logging atdebug!here and escalating inside the handler only when necessary.♻️ Sketch of deferred logging
Err(e) => { - warn!("Error moving file: {e}"); - handle_move_file_error(dropbox_client, from_path, into_path).await; + // Log at debug; handle_move_file_error will escalate if genuinely unexpected. + debug!("move_v2 returned error for {} → {}: {e}", from_path, into_path); + handle_move_file_error(dropbox_client, from_path, into_path).await; }🤖 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 197 - 198, The unconditional warn! before calling handle_move_file_error causes noisy warnings for benign idempotent-move cases; change the call site to await handle_move_file_error(dropbox_client, from_path, into_path).await and either (a) make handle_move_file_error return a Result/enum indicating whether the destination already matched (so the caller can log info/debug/warn appropriately) or (b) downgrade the call-site warn! to debug! and let handle_move_file_error escalate to warn!/error! only when it determines a real problem (e.g., on hash mismatch or non-recoverable errors); update the call and logging logic around warn!, handle_move_file_error, and the involved params (dropbox_client, from_path, into_path) accordingly.
149-150: Sequential metadata calls could be parallelised.Both
get_file_content_hashcalls are independent; running them sequentially doubles the round-trip latency on what is already an error-recovery path.♻️ Proposed refactor
- let from_content_hash = get_file_content_hash(dropbox_client, from_path).await; - let into_content_hash = get_file_content_hash(dropbox_client, into_path).await; + let (from_content_hash, into_content_hash) = tokio::join!( + get_file_content_hash(dropbox_client, from_path), + get_file_content_hash(dropbox_client, into_path), + );🤖 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 149 - 150, Both get_file_content_hash calls are independent and should run concurrently: replace the sequential awaits with a concurrent join such as let (from_content_hash, into_content_hash) = tokio::join!(get_file_content_hash(dropbox_client, from_path), get_file_content_hash(dropbox_client, into_path)); then adapt handling for the returned types (e.g., unpack Results or Options) and preserve existing error/recovery logic; reference the get_file_content_hash function and the from_content_hash / into_content_hash bindings when making the change.
🤖 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 151-181: The match on (from_content_hash, into_content_hash)
misinterprets None from get_file_content_hash and treats (None, Some(_)) as an
error; change get_file_content_hash's return to a tri-state enum (e.g.,
ContentHash::Hash(String) | ContentHash::Absent | ContentHash::Error(Error)) so
callers can distinguish "hash present", "path absent/moved", and "metadata fetch
error"; update the match in the handler (the match currently shown on
(from_content_hash, into_content_hash) and the branch that calls
files::delete_v2) to: compare Hash/Hash as before, treat (Absent, Hash) as
likely-success (log success/no-op and do not attempt delete), treat (Error, _)
or (_, Error) as metadata-fetch errors and include the error details in logs,
and only treat true Absent/Absent as an unexpected missing-file case; ensure
logs reference the enum variant to avoid phrasing like "file does not exist"
when the real issue was a metadata fetch failure.
---
Nitpick comments:
In `@download_manager/src/dropbox.rs`:
- Around line 197-198: The unconditional warn! before calling
handle_move_file_error causes noisy warnings for benign idempotent-move cases;
change the call site to await handle_move_file_error(dropbox_client, from_path,
into_path).await and either (a) make handle_move_file_error return a Result/enum
indicating whether the destination already matched (so the caller can log
info/debug/warn appropriately) or (b) downgrade the call-site warn! to debug!
and let handle_move_file_error escalate to warn!/error! only when it determines
a real problem (e.g., on hash mismatch or non-recoverable errors); update the
call and logging logic around warn!, handle_move_file_error, and the involved
params (dropbox_client, from_path, into_path) accordingly.
- Around line 149-150: Both get_file_content_hash calls are independent and
should run concurrently: replace the sequential awaits with a concurrent join
such as let (from_content_hash, into_content_hash) =
tokio::join!(get_file_content_hash(dropbox_client, from_path),
get_file_content_hash(dropbox_client, into_path)); then adapt handling for the
returned types (e.g., unpack Results or Options) and preserve existing
error/recovery logic; reference the get_file_content_hash function and the
from_content_hash / into_content_hash bindings when making the change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
download_manager/src/yt_dlp.rs (1)
83-83: Consider adding a brief comment explaining the 100 → 90 byte reduction.The change is correct and safe, but the rationale (avoiding Windows MAX_PATH / Dropbox path-length limits that can trigger relocation errors) is non-obvious and will be invisible to future maintainers without context.
💬 Suggested inline comment
- "{}/%(title).90B %(upload_date)s [%(webpage_url_domain)s %(id)s].%(ext)s", + // Title is capped at 90 bytes (reduced from 100) to keep the full + // path within Windows MAX_PATH limits when files are relocated on Dropbox. + "{}/%(title).90B %(upload_date)s [%(webpage_url_domain)s %(id)s].%(ext)s",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@download_manager/src/yt_dlp.rs` at line 83, Add a short inline comment next to the format string literal "{}/%(title).90B %(upload_date)s [%(webpage_url_domain)s %(id)s].%(ext)s" in yt_dlp.rs explaining why the title length was reduced from 100 to 90 bytes (to avoid Windows MAX_PATH/Dropbox path-length/relocation errors when creating long filenames), so future maintainers understand the rationale; place the comment immediately above or after that literal and keep it concise and specific.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@download_manager/src/yt_dlp.rs`:
- Line 83: Add a short inline comment next to the format string literal
"{}/%(title).90B %(upload_date)s [%(webpage_url_domain)s %(id)s].%(ext)s" in
yt_dlp.rs explaining why the title length was reduced from 100 to 90 bytes (to
avoid Windows MAX_PATH/Dropbox path-length/relocation errors when creating long
filenames), so future maintainers understand the rationale; place the comment
immediately above or after that literal and keep it concise and specific.
TyRoXx
force-pushed
the
relocate-error
branch
from
96f37d8 to
5de050d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
download_manager/src/dropbox.rs (1)
149-168: Sequential hash fetches can be parallelised.The two
get_file_content_hashcalls are independent; running them concurrently halves the round-trip latency for the happy path.⚡ Proposed parallelisation
- let from_content_hash = match get_file_content_hash(dropbox_client, from_path).await { - Some(hash) => hash, - None => { - error!( - "Could not get content hash for source file {}, cannot handle move error", - from_path - ); - return; - } - }; - let into_content_hash = match get_file_content_hash(dropbox_client, into_path).await { - Some(hash) => hash, - None => { - error!( - "Could not get content hash for destination file {}, cannot handle move error", - into_path - ); - return; - } - }; + let (from_result, into_result) = tokio::join!( + get_file_content_hash(dropbox_client, from_path), + get_file_content_hash(dropbox_client, into_path), + ); + let from_content_hash = match from_result { + Some(hash) => hash, + None => { + error!( + "Could not get content hash for source file {}, cannot handle move error", + from_path + ); + return; + } + }; + let into_content_hash = match into_result { + Some(hash) => hash, + None => { + error!( + "Could not get content hash for destination file {}, cannot handle move error", + into_path + ); + return; + } + };🤖 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 149 - 168, The two independent awaits for get_file_content_hash (producing from_content_hash and into_content_hash) should be run concurrently to reduce latency: spawn both futures together (e.g., using tokio::join! or futures::join!) on the calls get_file_content_hash(dropbox_client, from_path) and get_file_content_hash(dropbox_client, into_path), then inspect both results and log and return if either is None; preserve the existing error messages but reference which path failed (from_path or into_path) when returning. Use the existing symbols get_file_content_hash, dropbox_client, from_path, into_path, from_content_hash and into_content_hash when implementing this change.
🤖 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 123-129: The match arm handling files::Metadata::Deleted is
unreachable because GetMetadataArg::new() isn't configured with include_deleted;
either remove the files::Metadata::Deleted branch or enable deleted results by
setting include_deleted on the GetMetadataArg construction (call
.with_include_deleted(true) on the GetMetadataArg::new() used when fetching
metadata). If you keep the branch, update the GetMetadataArg::new() call that
precedes the metadata fetch to chain .with_include_deleted(true); if you remove
it, delete the files::Metadata::Deleted arm and its associated variables
(deleted_metadata, file_path) to eliminate dead code.
---
Duplicate comments:
In `@download_manager/src/dropbox.rs`:
- Around line 98-142: get_file_content_hash currently returns Option<String>
which collapses API/network errors and "file absent" into None; introduce a
tri-state enum (e.g., enum FileHashResult { Hash(String), Absent,
Error(anyhow::Error) }) and change get_file_content_hash signature to return
that enum, mapping: files::Metadata::File + Some(content_hash) ->
FileHashResult::Hash, Folder/Deleted/missing-hash -> FileHashResult::Absent, and
any call/HTTP errors (the Err branch from files::get_metadata) ->
FileHashResult::Error with the underlying error preserved; update callers
(notably handle_move_file_error and any logic that currently treats None as
absent) to match on the new enum and only treat Absent as "path gone", while
Error should be handled/logged as an API/network failure.
- Around line 149-157: The code in handle_move_file_error currently bails out
when get_file_content_hash(from_path) returns None, which masks the
likely-success case where the source was removed by a completed move; instead,
when from_content_hash is None call get_file_content_hash for the destination
(into_content_hash) and if into_content_hash is Some(hash) treat this as a
probable successful move: log a clear message (e.g., "source missing but
destination has content hash, likely move succeeded") and return rather than
logging "cannot handle move error" and aborting; keep using the existing
functions get_file_content_hash and into_content_hash (and update messages
around handle_move_file_error and the earlier error log) so we distinguish
absent-source+present-dest from true metadata-fetch failures.
---
Nitpick comments:
In `@download_manager/src/dropbox.rs`:
- Around line 149-168: The two independent awaits for get_file_content_hash
(producing from_content_hash and into_content_hash) should be run concurrently
to reduce latency: spawn both futures together (e.g., using tokio::join! or
futures::join!) on the calls get_file_content_hash(dropbox_client, from_path)
and get_file_content_hash(dropbox_client, into_path), then inspect both results
and log and return if either is None; preserve the existing error messages but
reference which path failed (from_path or into_path) when returning. Use the
existing symbols get_file_content_hash, dropbox_client, from_path, into_path,
from_content_hash and into_content_hash when implementing this change.
5de050d to
fe3b1f9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fuzz.sh (2)
6-6: Prefer$(…)over backtick command substitution.Backticks are considered deprecated in favour of the POSIX
$(…)form, which is cleaner, nestable, and easier to read.♻️ Proposed fix
-JOBS=`nproc` +JOBS=$(nproc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz.sh` at line 6, Replace the deprecated backtick command substitution used to set JOBS with the POSIX form: change the assignment that currently uses JOBS=`nproc` to use the $(...) construct so it becomes JOBS=$(nproc); update the line where JOBS is defined in fuzz.sh (referencing the JOBS variable and nproc invocation) accordingly.
7-7: 5 seconds per target may be too short for meaningful fuzzing.At 5 s, libFuzzer barely completes initialisation and corpus loading before hitting the time limit, so the step is unlikely to catch anything beyond a crash-on-startup. If the intent is a quick CI sanity check that is acceptable, but it would be worth documenting that clearly (e.g. a comment in the script or the PR) so that reviewers know meaningful fuzzing is expected to happen in a separate, longer-running job.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz.sh` at line 7, The TIME_PER_TARGET=5 setting in fuzz.sh is too short for meaningful fuzzing; update fuzz.sh to increase TIME_PER_TARGET to a longer default (for example 60 or 300 seconds) or make it configurable via an environment variable, and add a clear comment above the TIME_PER_TARGET declaration documenting that the short value is only for fast CI sanity checks and that full/long-running fuzzing should run in a separate job; modify the TIME_PER_TARGET line and the nearby comment in fuzz.sh accordingly so reviewers understand intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fuzz.sh`:
- Line 6: Replace the deprecated backtick command substitution used to set JOBS
with the POSIX form: change the assignment that currently uses JOBS=`nproc` to
use the $(...) construct so it becomes JOBS=$(nproc); update the line where JOBS
is defined in fuzz.sh (referencing the JOBS variable and nproc invocation)
accordingly.
- Line 7: The TIME_PER_TARGET=5 setting in fuzz.sh is too short for meaningful
fuzzing; update fuzz.sh to increase TIME_PER_TARGET to a longer default (for
example 60 or 300 seconds) or make it configurable via an environment variable,
and add a clear comment above the TIME_PER_TARGET declaration documenting that
the short value is only for fast CI sanity checks and that full/long-running
fuzzing should run in a separate job; modify the TIME_PER_TARGET line and the
nearby comment in fuzz.sh accordingly so reviewers understand intent.
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
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 by CodeRabbit
Bug Fixes
Behaviour Change
Chores