Add part of Safe clean #45
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a fully implemented ChangesClean Command Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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)
crates/dustfril-core/src/models/cleanup_plan.rs (1)
3-7: 🏗️ Heavy liftAvoid storing a mutable aggregate beside its source list.
candidatesandreclaimable_size_bytescan drift apart because both are independently mutable. That turns into incorrect cleanup totals in previews and summaries. Prefer deriving the total fromcandidatesor enforcing the invariant via a constructor/accessor.One way to enforce the invariant
use crate::models::CleanupCandidate; #[derive(Debug, Default)] pub struct CleanupPlan { pub candidates: Vec<CleanupCandidate>, - - pub reclaimable_size_bytes: u64, } + +impl CleanupPlan { + pub fn reclaimable_size_bytes(&self) -> u64 { + self.candidates + .iter() + .map(|candidate| candidate.size_bytes) + .sum() + } +}🤖 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/dustfril-core/src/models/cleanup_plan.rs` around lines 3 - 7, The struct CleanupPlan currently stores a mutable aggregate reclaimable_size_bytes alongside the source list candidates causing potential drift; remove the public reclaimable_size_bytes field and instead derive the total on demand by adding a method (e.g., CleanupPlan::reclaimable_size_bytes() or total_reclaimable_bytes()) that iterates candidates and sums their sizes, or alternatively make reclaimable_size_bytes private and populate/maintain it only via a constructor and mutators that update it whenever candidates changes; update any call sites that read reclaimable_size_bytes to call the new accessor so the invariant is enforced by computation or controlled mutation in CleanupPlan.crates/dustfril-core/src/cleaner/executor.rs (1)
13-23: 🏗️ Heavy liftCleanup executor
matchis exhaustive; focus on better failure reporting instead.
ArtifactTypehas onlyTarget,CargoRegistry, andCargoGit, so the executor’smatchcovers all variants (future enum additions will fail to compile rather than being silently skipped).- Remaining gap:
remove_dir_allerrors are collapsed tofailed_pathswithout any error details, and the CLI only prints the number of failed paths (not which ones). Consider printingfailed_pathsand/or logging the underlying 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/dustfril-core/src/cleaner/executor.rs` around lines 13 - 23, The cleanup executor currently treats all fs::remove_dir_all failures the same by only pushing candidate.path into result.failed_paths; update the failure handling in the match arm that inspects candidate.artifact_type so you capture and surface the underlying IO error (from fs::remove_dir_all) instead of discarding it: on Err(e) include the error message alongside the path (e.g., extend result.failed_paths to carry path+error or push a structured failure record) and ensure the CLI/logging prints these failed paths and their errors; modify references around candidate.path, fs::remove_dir_all, result.failed_paths, result.deleted_paths, and result.freed_size_bytes accordingly.
🤖 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 `@apps/dustfril-cli/src/cli.rs`:
- Around line 24-27: Add a help string for the dry_run flag on the CleanArgs
struct so the CLI help shows why it exists; update the dry_run field
(CleanArgs::dry_run) to include either a field doc comment or an #[arg(help =
"...")] attribute that explains it performs a non-destructive dry run and does
not actually delete files (e.g., "Perform a dry run; show what would be removed
without deleting anything"), leaving the flag name #[arg(long)] unchanged.
---
Nitpick comments:
In `@crates/dustfril-core/src/cleaner/executor.rs`:
- Around line 13-23: The cleanup executor currently treats all
fs::remove_dir_all failures the same by only pushing candidate.path into
result.failed_paths; update the failure handling in the match arm that inspects
candidate.artifact_type so you capture and surface the underlying IO error (from
fs::remove_dir_all) instead of discarding it: on Err(e) include the error
message alongside the path (e.g., extend result.failed_paths to carry path+error
or push a structured failure record) and ensure the CLI/logging prints these
failed paths and their errors; modify references around candidate.path,
fs::remove_dir_all, result.failed_paths, result.deleted_paths, and
result.freed_size_bytes accordingly.
In `@crates/dustfril-core/src/models/cleanup_plan.rs`:
- Around line 3-7: The struct CleanupPlan currently stores a mutable aggregate
reclaimable_size_bytes alongside the source list candidates causing potential
drift; remove the public reclaimable_size_bytes field and instead derive the
total on demand by adding a method (e.g., CleanupPlan::reclaimable_size_bytes()
or total_reclaimable_bytes()) that iterates candidates and sums their sizes, or
alternatively make reclaimable_size_bytes private and populate/maintain it only
via a constructor and mutators that update it whenever candidates changes;
update any call sites that read reclaimable_size_bytes to call the new accessor
so the invariant is enforced by computation or controlled mutation in
CleanupPlan.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60da3d46-3506-4f69-a1b5-da1511fa4650
📒 Files selected for processing (11)
apps/dustfril-cli/src/cli.rsapps/dustfril-cli/src/commands/clean.rsapps/dustfril-cli/src/main.rscrates/dustfril-core/src/cleaner/executor.rscrates/dustfril-core/src/cleaner/mod.rscrates/dustfril-core/src/cleaner/plan.rscrates/dustfril-core/src/cleaner/tests.rscrates/dustfril-core/src/models/cleanup_candidate.rscrates/dustfril-core/src/models/cleanup_plan.rscrates/dustfril-core/src/models/cleanup_result.rscrates/dustfril-core/src/models/mod.rs
Comment on lines
+24
to
+27
| #[derive(Args)] | ||
| pub struct CleanArgs { | ||
| #[arg(long)] | ||
| pub dry_run: bool, |
There was a problem hiding this comment.
Add help text for --dry-run.
This flag is central to the safety story, but without a field doc comment it shows up in --help with no explanation.
Suggested fix
#[derive(Args)]
pub struct CleanArgs {
+ /// Preview cleanup operations without deleting files.
#[arg(long)]
pub dry_run: bool,
}📝 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
| #[derive(Args)] | |
| pub struct CleanArgs { | |
| #[arg(long)] | |
| pub dry_run: bool, | |
| #[derive(Args)] | |
| pub struct CleanArgs { | |
| /// Preview cleanup operations without deleting files. | |
| #[arg(long)] | |
| pub dry_run: bool, | |
| } |
🤖 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 `@apps/dustfril-cli/src/cli.rs` around lines 24 - 27, Add a help string for the
dry_run flag on the CleanArgs struct so the CLI help shows why it exists; update
the dry_run field (CleanArgs::dry_run) to include either a field doc comment or
an #[arg(help = "...")] attribute that explains it performs a non-destructive
dry run and does not actually delete files (e.g., "Perform a dry run; show what
would be removed without deleting anything"), leaving the flag name #[arg(long)]
unchanged.
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.
What
Safe cleaning and additional cleaning commands
Create cleanup plan and result
Add clean command
Add dry run support
Add confirmation prompt
Add cleanup summary and tests
Closes #22
Closes #23
Closes #24
Closes #25
Checklist
Required
Functional Validation
Documentation
Safety
Summary by CodeRabbit
New Features
Tests