Fix incomplete plugin directory deletion on uninstall#4250
Fix incomplete plugin directory deletion on uninstall#4250Jack251970 wants to merge 7 commits intoFlow-Launcher:devfrom
Conversation
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
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.
…letion Fix incomplete plugin directory deletion on uninstall
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughAdds a robust directory-deletion routine and switches plugin cleanup to use it; if deletion partially fails, a marker file is written and a warning is logged to retry cleanup on next startup. Changes
Sequence DiagramsequenceDiagram
participant PC as PluginConfig
participant FD as FilesFolders.TryDeleteDirectoryRobust
participant FS as File System
participant DL as DataLocation
participant PA as PublicApi (Logger)
PC->>FD: request delete directory(path)
FD->>FS: enumerate files & subdirs
loop per-file retries
FD->>FS: remove read-only attr if present
FD->>FS: attempt delete file
alt delete succeeds
FS-->>FD: file removed
else retryable failure
FD-->>FS: wait retryDelay and retry
else non-retryable failure
FS-->>FD: failure
end
end
FD->>FS: delete empty subdirectories (deep → shallow)
FD->>FS: delete root directory
alt all deleted
FS-->>FD: success
FD-->>PC: return true
else partial/failure
FS-->>FD: partial/failure
FD-->>PC: return false
end
alt deletion returned false
PC->>DL: create PluginDeleteFile marker in directory (if missing)
PC->>PA: LogWarn about deferred deletion
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 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 pull request enhances plugin directory deletion in Flow Launcher by introducing a robust deletion mechanism that handles locked and read-only files more gracefully. The changes ensure that incomplete deletions are retried on subsequent application startups.
Changes:
- Added
TryDeleteDirectoryRobustmethod with retry logic for handling locked/read-only files during deletion - Updated plugin configuration to use the new robust deletion method and implement marker file recreation for retry logic
- Added comprehensive unit tests covering various deletion scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs | Implements new TryDeleteDirectoryRobust method with retry logic and read-only file handling |
| Flow.Launcher.Core/Plugin/PluginConfig.cs | Updates deletion logic to use robust method, adds marker file recreation and warning logging for incomplete deletions |
| Flow.Launcher.Test/FilesFoldersTest.cs | Adds unit tests for various deletion scenarios including empty directories, files, nested structures, and read-only files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs">
<violation number="1" location="Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs:209">
P2: `Directory.Delete` will throw an `IOException` if the directory itself is marked ReadOnly. The current implementation clears the ReadOnly attribute for files but not for directories. If a directory is ReadOnly, this method will permanently fail to delete it, causing infinite retries on subsequent startups.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Simplified the warning log when a directory is not fully deleted, removing extra details about remaining files and retrying deletion. Marker file recreation logic is unchanged.
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: PatternIf the flagged items are 🤯 false positivesIf items relate to a ...
|
Test: