Improve code quality #4257
Improve code quality #4257
Conversation
Co-authored-by: Jack251970 <[email protected]>
Co-authored-by: Jack251970 <[email protected]>
Refactored the process for recreating the marker file when a plugin directory is not fully deleted. Removed unnecessary try-catch and nested checks, now directly checking for file existence and creating it if missing. This streamlines the code and removes redundant error handling.
Raised retry delay for directory deletion from 100ms to 200ms to improve reliability when files are locked or in use.
Updated the log message for incomplete directory deletion to be clearer and more concise. The warning is now logged before recreating the marker file, and no longer mentions retrying deletion or remaining files/folders.
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe PR introduces a robust directory deletion utility method with retry logic and fallback mechanism, replacing a simple direct deletion call in plugin cleanup. On failure, pending deletions are marked for retry at next startup via a marker file. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a more resilient directory deletion helper and switches plugin cleanup to use it, adding unit tests to validate common deletion scenarios.
Changes:
- Added
FilesFolders.TryDeleteDirectoryRobust(...)with per-file retry behavior and read-only attribute handling. - Updated plugin deletion flow to use the robust delete method and re-create the “delete marker” when deletion is incomplete.
- Added NUnit tests covering deletion of non-existent, empty, nested, and read-only-file directories.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Flow.Launcher.Test/FilesFoldersTest.cs | Adds new NUnit tests for the new robust directory deletion helper. |
| Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs | Implements TryDeleteDirectoryRobust with retry logic and recursive cleanup. |
| Flow.Launcher.Core/Plugin/PluginConfig.cs | Uses the new robust delete for plugin directory cleanup and attempts to re-create deletion marker on partial failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool fileDeleted = false; | ||
| for (int attempt = 0; attempt <= maxRetries; attempt++) | ||
| { |
There was a problem hiding this comment.
The retry loop uses attempt <= maxRetries, which results in maxRetries + 1 total attempts. This conflicts with the XML doc wording (“maximum number of retry attempts”). Either change the loop to attempt < maxRetries (treating maxRetries as total attempts) or update the doc to clarify it means “retries after the first attempt.”
| // First, try to delete all files in the directory tree | ||
| var files = Directory.GetFiles(path, "*", SearchOption.AllDirectories); | ||
| foreach (var file in files) |
There was a problem hiding this comment.
Directory.GetFiles(..., AllDirectories) eagerly materializes the entire file list and can be very memory-heavy for large directories. Consider switching to Directory.EnumerateFiles (and similarly for directories) to stream entries and reduce allocations during deletion.
| if (!File.Exists(markerFilePath)) | ||
| { | ||
| File.WriteAllText(markerFilePath, string.Empty); | ||
| } |
There was a problem hiding this comment.
When deletion is not fully successful, the code attempts to recreate the marker file under directory without first ensuring the directory still exists. If the root directory was actually deleted (or moved) despite fullyDeleted being false (e.g., due to an exception after deletion), File.WriteAllText will throw DirectoryNotFoundException and the marker won’t be recreated. Guard with Directory.Exists(directory) before writing the marker (and skip/re-log if it no longer exists).
| if (!File.Exists(markerFilePath)) | |
| { | |
| File.WriteAllText(markerFilePath, string.Empty); | |
| } | |
| if (Directory.Exists(directory) && !File.Exists(markerFilePath)) | |
| { | |
| File.WriteAllText(markerFilePath, string.Empty); | |
| } | |
| else if (!Directory.Exists(directory)) | |
| { | |
| PublicApi.Instance.LogWarn(ClassName, | |
| $"Directory <{directory}> no longer exists when attempting to recreate delete marker file."); | |
| } |
| public void TryDeleteDirectoryRobust_WhenDirectoryIsEmpty_DeletesSuccessfully() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } |
There was a problem hiding this comment.
These tests create temp directories/files but don’t have a finally cleanup path if an assertion fails or an unexpected exception is thrown. That can leave artifacts in the temp folder and make subsequent runs flaky. Consider wrapping each test body with try/finally (or using [TearDown]) to delete the temp directory if it still exists, and for the read-only case restore attributes before cleanup if needed.
Summary by cubic
Make plugin directory deletion more reliable by adding robust retry logic and recreating a deletion marker when cleanup is incomplete. This reduces uninstall failures caused by locked or read-only files and ensures cleanup completes on the next startup.
Written for commit 0acda2c. Summary will update on new commits.