打开文件夹测试 #19
打开文件夹测试 #19
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant MoveModule
participant OpenModule
participant Utils
User->>UI: Click "Open" button
UI->>OpenModule: open_folder(folder_path)
OpenModule->>OpenModule: Check if path exists
OpenModule->>User: Return error if not exists
OpenModule->>OpenModule: Execute OS-specific command
OpenModule->>User: Return success or error
User->>UI: Click "Move" button
UI->>MoveModule: show_move_dialog(ctx, folder_name, source_path)
MoveModule->>MoveModule: Move folder logic
MoveModule->>User: Return success or error
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/move_module.rs (2)
18-24: Replaceprintln!statements with proper logging or remove themThe
println!statements in lines 18, 20, 22, 24, 36, 39, and 42 are used for debugging purposes. It's recommended to replace these with appropriate logging using theloggermodule (e.g.,logger::log_info) or remove them if they're no longer needed. This helps maintain consistent logging practices and avoids cluttering the console output.Also applies to: 36-39, 42-42
50-81: Improve progress calculation to account for files in subdirectoriesIn the
move_folderfunction, the progress is calculated based ontotal_files = entries.len(), which only counts the entries in the current directory level. When processing subdirectories recursively (line 71), thetotal_filesdoes not include files in these subdirectories, leading to inaccurate progress reporting.Consider modifying the implementation to calculate the total number of files to be moved, including all files in subdirectories, before starting the move operation. This will provide more accurate progress updates.
src/open.rs (2)
3-3: Uncomment the logger import and utilize logging for error handlingThe
loggerimport is commented out in line 3://use crate::logger;Consider uncommenting this line and using the
loggermodule to log errors and important events. Integrating logging will help in tracking the application's behavior and ease debugging.
21-32: Improve error handling and provide user feedbackIn the
matchstatement handling the command execution result (lines 21-32), consider logging the errors and providing more detailed user feedback. This could involve:
- Logging the specific error messages using the
loggermodule.- Including the external command's standard error output in the error messages for better diagnostics.
Example modification:
match status { Ok(s) => { if cfg!(target_os = "windows") { Ok(()) } else if s.success() { Ok(()) } else { let stderr = String::from_utf8_lossy(&s.stderr); logger::log_error(&format!("打开文件夹失败，状态码: {}，错误信息: {}", s, stderr)); Err(format!("打开文件夹失败，状态码: {}", s)) } } Err(e) => { logger::log_error(&format!("执行打开命令失败: {}", e)); Err(format!("执行打开命令失败: {}", e)) } }This enhances error reporting and assists in troubleshooting.
src/utils.rs (1)
37-46: Consider optimizing memory usage for large directories.The current implementation loads all file hashes into memory before comparison. For directories with many files, this could lead to high memory usage.
Consider this streaming comparison approach:
pub fn compare_dirs_hash(source: &Path, target: &Path) -> Result<bool, std::io::Error> { - let source_hashes: Vec<_> = fs::read_dir(source)? - .map(|entry| hash_file(&entry?.path())) - .collect::<Result<_, _>>()?; - let target_hashes: Vec<_> = fs::read_dir(target)? - .map(|entry| hash_file(&entry?.path())) - .collect::<Result<_, _>>()?; - - Ok(source_hashes == target_hashes) + let mut source_entries: Vec<_> = fs::read_dir(source)?.collect::<Result<_, _>>()?; + let mut target_entries: Vec<_> = fs::read_dir(target)?.collect::<Result<_, _>>()?; + + if source_entries.len() != target_entries.len() { + return Ok(false); + } + + source_entries.sort_by_key(|entry| entry.path()); + target_entries.sort_by_key(|entry| entry.path()); + + for (source_entry, target_entry) in source_entries.iter().zip(target_entries.iter()) { + let source_hash = hash_file(&source_entry.path())?; + let target_hash = hash_file(&target_entry.path())?; + if source_hash != target_hash { + return Ok(false); + } + } + Ok(true) }src/ui.rs (1)
187-212: Replace println statements with logger calls for consistency.The move functionality implementation mixes println statements with logger calls. For consistency and better debugging, use the logger throughout.
- println!("show_move_dialog is called"); + logger::log_info("Showing move dialog"); - println!("Confirmed move to {:?}", target_path); + logger::log_info(&format!("Confirmed move to {:?}", target_path)); - println!("移动进度: {:.2}%", p * 100.0); + logger::log_info(&format!("移动进度: {:.2}%", p * 100.0));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Cargo.toml(2 hunks)README.md(1 hunks)src/delete.rs(1 hunks)src/ignore.rs(1 hunks)src/main.rs(1 hunks)src/move_module.rs(1 hunks)src/open.rs(1 hunks)src/ui.rs(3 hunks)src/utils.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/ignore.rs
🔇 Additional comments (5)
Cargo.toml (1)
3-3: Version update and new dependencies reviewed
The version bump to 1.0.3-b1 is appropriate given the new features added. The addition of rfd = "0.15.1" and sha2 = "0.10" dependencies is consistent with the implemented functionalities in the codebase.
Also applies to: 14-15
src/main.rs (1)
9-10: New modules move_module and open integrated correctly
The addition of mod move_module; and mod open; in lines 9-10 successfully integrates the new functionalities into the main application.
src/delete.rs (1)
2-2: Cleanup of unused import
The removal of use std::path::Path; in line 2 tidies up the code by eliminating an unused import. This enhances code readability.
src/utils.rs (1)
30-35: LGTM! Efficient implementation of file hashing.
The implementation properly handles I/O errors and efficiently streams file content using std::io::copy.
README.md (1)
78-80: LGTM! Documentation updates accurately reflect implemented features.
The TODO list has been properly updated to mark completed features with appropriate version references.
| let total_files = entries.len(); | ||
| let mut copied_files = 0; | ||
|
|
||
| fs::create_dir_all(target).expect("Failed to create target directory"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors without panicking when creating the target directory
Using expect in line 61:
fs::create_dir_all(target).expect("Failed to create target directory");will cause the program to panic if the directory creation fails. Instead, it's better to handle this error gracefully by propagating the error using the ? operator:
fs::create_dir_all(target)?;This way, the error can be returned to the caller, and the user can be notified appropriately.
| pub fn verify_and_create_symlink(source: &Path, target: &Path) -> io::Result<()> { | ||
| if !utils::compare_dirs_hash(source, target)? { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::Other, | ||
| "源文件夹与目标文件夹哈希值不匹配", | ||
| )); | ||
| } | ||
|
|
||
| fs::remove_dir_all(source)?; | ||
|
|
||
| let output = std::process::Command::new("cmd") | ||
| .args(["/C", "mklink", "/D", &source.display().to_string(), &target.display().to_string()]) | ||
| .output()?; | ||
|
|
||
| let output_str = String::from_utf8_lossy(&output.stdout); | ||
| if output_str.contains("<<===>>") { | ||
| Ok(()) | ||
| } else { | ||
| Err(io::Error::new( | ||
| io::ErrorKind::Other, | ||
| "创建符号链接失败", | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Rust's standard library functions for creating symbolic links
In the verify_and_create_symlink function (lines 83-106), you're creating a symbolic link by invoking cmd and running mklink through the command line. This approach has several drawbacks:
- Platform Dependency: The current method is Windows-specific and won't work on other operating systems.
- Administrative Privileges:
mklinktypically requires administrative privileges, which might not be available and could lead to failures. - Error Handling: Parsing command output to determine success is error-prone and less reliable.
Consider using Rust's standard library functions to create symbolic links, which are more robust and cross-platform compatible:
- Windows: Use
std::os::windows::fs::symlink_dir. - Unix-like Systems: Use
std::os::unix::fs::symlink.
Here's how you might modify your code:
#[cfg(target_os = "windows")]
use std::os::windows::fs::symlink_dir;
#[cfg(unix)]
use std::os::unix::fs::symlink;
pub fn verify_and_create_symlink(source: &Path, target: &Path) -> io::Result<()> {
if !utils::compare_dirs_hash(source, target)? {
return Err(io::Error::new(
io::ErrorKind::Other,
"源文件夹与目标文件夹哈希值不匹配",
));
}
fs::remove_dir_all(source)?;
#[cfg(target_os = "windows")]
symlink_dir(target, source)?;
#[cfg(unix)]
symlink(target, source)?;
Ok(())
}This approach eliminates the need to run external commands and provides better error handling.
| // 操作区内逻辑，新增 "打开" 按钮 | ||
| if ui.button("打开").clicked() { | ||
| if let Some(base_path) = utils::get_appdata_dir(&self.selected_appdata_folder) { | ||
| let full_path = base_path.join(folder); | ||
| match open::open_folder(&full_path) { | ||
| Ok(_) => { | ||
| println!("成功打开文件夹: {}", full_path.display()); | ||
| logger::log_info(&format!("成功打开文件夹: {}", full_path.display())); | ||
| } | ||
| Err(err) => { | ||
| eprintln!("无法打开文件夹: {}", err); | ||
| logger::log_error(&format!("无法打开文件夹: {}", err)); | ||
| } | ||
| } | ||
| } else { | ||
| eprintln!("无法获取 {} 文件夹路径", self.selected_appdata_folder); | ||
| logger::log_error(&format!("无法获取 {} 文件夹路径", self.selected_appdata_folder)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add path sanitization for folder opening.
The current implementation directly joins paths without sanitization, which could potentially lead to path traversal vulnerabilities if the folder name contains malicious components like "../".
Add path sanitization:
if ui.button("打开").clicked() {
if let Some(base_path) = utils::get_appdata_dir(&self.selected_appdata_folder) {
- let full_path = base_path.join(folder);
+ let sanitized_folder = folder.replace("..", "").replace("/", "").replace("\\", "");
+ let full_path = base_path.join(sanitized_folder);
match open::open_folder(&full_path) {
Ok(_) => {
- println!("成功打开文件夹: {}", full_path.display());
logger::log_info(&format!("成功打开文件夹: {}", full_path.display()));
}
Err(err) => {
- eprintln!("无法打开文件夹: {}", err);
logger::log_error(&format!("无法打开文件夹: {}", err));
}
}
} else {
- eprintln!("无法获取 {} 文件夹路径", self.selected_appdata_folder);
logger::log_error(&format!("无法获取 {} 文件夹路径", self.selected_appdata_folder));
}
}📝 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 ui.button("打开").clicked() { | |
| if let Some(base_path) = utils::get_appdata_dir(&self.selected_appdata_folder) { | |
| let full_path = base_path.join(folder); | |
| match open::open_folder(&full_path) { | |
| Ok(_) => { | |
| println!("成功打开文件夹: {}", full_path.display()); | |
| logger::log_info(&format!("成功打开文件夹: {}", full_path.display())); | |
| } | |
| Err(err) => { | |
| eprintln!("无法打开文件夹: {}", err); | |
| logger::log_error(&format!("无法打开文件夹: {}", err)); | |
| } | |
| } | |
| } else { | |
| eprintln!("无法获取 {} 文件夹路径", self.selected_appdata_folder); | |
| logger::log_error(&format!("无法获取 {} 文件夹路径", self.selected_appdata_folder)); | |
| } | |
| } | |
| // 操作区内逻辑，新增 "打开" 按钮 | |
| if ui.button("打开").clicked() { | |
| if let Some(base_path) = utils::get_appdata_dir(&self.selected_appdata_folder) { | |
| let sanitized_folder = folder.replace("..", "").replace("/", "").replace("\\", ""); | |
| let full_path = base_path.join(sanitized_folder); | |
| match open::open_folder(&full_path) { | |
| Ok(_) => { | |
| logger::log_info(&format!("成功打开文件夹: {}", full_path.display())); | |
| } | |
| Err(err) => { | |
| logger::log_error(&format!("无法打开文件夹: {}", err)); | |
| } | |
| } | |
| } else { | |
| logger::log_error(&format!("无法获取 {} 文件夹路径", self.selected_appdata_folder)); | |
| } | |
| } |
Signed-off-by: 陈生杂物房 <88823709+TC999@users.noreply.github.com>
调用系统资源管理器打开文件夹
Summary by CodeRabbit
Release Notes for AppDataCleaner v1.0.3-b1
New Features
Bug Fixes
Documentation
Chores