fix: explain reset-data Windows file locks#2395
Conversation
📝 WalkthroughWalkthroughThis PR improves error handling during the ChangesWindows file-lock error handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 1
🤖 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 `@app/src-tauri/src/lib.rs`:
- Around line 3407-3414: The test currently asserts a platform-specific
hardcoded path string "/tmp/openhuman"; change the assertion to use the same
Path display value used when building the message so it is platform-agnostic:
construct the test Path (the same one passed into reset_local_data_delete_error)
and use its display() (or its to_string/display representation) in the assert!
that checks msg.starts_with(...) and any other assertions that compare the path
text, referencing reset_local_data_delete_error and the Path you create in the
test so the test works on Windows and Unix.
🪄 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: 7361a977-f8c1-4ae0-859a-431132673e67
📒 Files selected for processing (1)
app/src-tauri/src/lib.rs
| let msg = reset_local_data_delete_error( | ||
| "current openhuman dir", | ||
| std::path::Path::new("/tmp/openhuman"), | ||
| &err, | ||
| ); | ||
|
|
||
| assert!(msg.starts_with("Failed to remove current openhuman dir at /tmp/openhuman:")); | ||
| assert!(!msg.contains("Close all OpenHuman windows and try again")); |
There was a problem hiding this comment.
Make the generic-message test path assertion platform-agnostic.
The assertion hardcodes /tmp/openhuman, which is brittle on Windows path rendering. Assert against the same Path display value used to build the message.
Suggested patch
fn reset_local_data_delete_error_keeps_generic_message_for_other_errors() {
let err = std::io::Error::from(std::io::ErrorKind::PermissionDenied);
+ let path = std::path::Path::new("/tmp/openhuman");
let msg = reset_local_data_delete_error(
"current openhuman dir",
- std::path::Path::new("/tmp/openhuman"),
+ path,
&err,
);
- assert!(msg.starts_with("Failed to remove current openhuman dir at /tmp/openhuman:"));
+ let expected_prefix = format!(
+ "Failed to remove current openhuman dir at {}:",
+ path.display()
+ );
+ assert!(msg.starts_with(&expected_prefix));
assert!(!msg.contains("Close all OpenHuman windows and try again"));
}🤖 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 `@app/src-tauri/src/lib.rs` around lines 3407 - 3414, The test currently
asserts a platform-specific hardcoded path string "/tmp/openhuman"; change the
assertion to use the same Path display value used when building the message so
it is platform-agnostic: construct the test Path (the same one passed into
reset_local_data_delete_error) and use its display() (or its to_string/display
representation) in the assert! that checks msg.starts_with(...) and any other
assertions that compare the path text, referencing reset_local_data_delete_error
and the Path you create in the test so the test works on Windows and Unix.
graycyrus
left a comment
There was a problem hiding this comment.
Looks good, nice work!
Summary
Tests
Addresses #1615
Summary by CodeRabbit
Bug Fixes
Tests