Fix prevent zombie explorer.exe processes when opening folders #3552
Conversation
… process start logic
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PublicAPIInstance
participant Shell/Process
participant CustomFileManager
Caller->>PublicAPIInstance: OpenDirectory(directoryPath, fileNameOrFilePath)
PublicAPIInstance->>PublicAPIInstance: Resolve targetPath
PublicAPIInstance->>PublicAPIInstance: Get custom file explorer path
alt Using default Windows Explorer
PublicAPIInstance->>Shell/Process: Start(targetPath, UseShellExecute=true)
else Using custom file manager
PublicAPIInstance->>CustomFileManager: Start(customExplorerPath, args)
end
Assessment against linked issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
346-359: Consider adding error handling for Process.StartThe code starts processes without any try-catch blocks, which could lead to unhandled exceptions if the paths are invalid or there are permission issues.
Consider adding error handling:
- Process.Start(psi); + try + { + Process.Start(psi); + } + catch (Exception e) + { + LogException(nameof(PublicAPIInstance), $"Failed to open directory: {directoryPath}", e); + ShowMsgError(GetTranslation("failedToOpenDirectory")); + }Also apply similar error handling to the explorer.exe case above.
335-336: Consider more robust explorer.exe detectionThe current implementation checks if the lowercase filename is "explorer", which works for typical cases but might be improved.
Consider using a case-insensitive string comparison instead of normalizing to lowercase:
-var explorerPath = explorerInfo.Path.Trim().ToLowerInvariant(); - -// If explorer.exe, ignore and pass only the path to Shell -if (Path.GetFileNameWithoutExtension(explorerPath) == "explorer") +var explorerPath = explorerInfo.Path.Trim(); + +// If explorer.exe, ignore and pass only the path to Shell +if (string.Equals(Path.GetFileNameWithoutExtension(explorerPath), "explorer", StringComparison.OrdinalIgnoreCase))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/PublicAPIInstance.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher/PublicAPIInstance.cs (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
OpenDirectory(301-301)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
OpenDirectory(163-166)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/PublicAPIInstance.cs
[warning] 49-52: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 145-174: Spell check warning: VSTHRD is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher/PublicAPIInstance.cs (3)
319-330: LGTM: Improved path resolution logicThe new path resolution logic correctly handles both null paths and absolute/relative file paths, ensuring proper path combining behavior.
334-344: Excellent fix for the zombie explorer.exe processesThis solution elegantly addresses the issue by bypassing direct explorer.exe process creation when the default file explorer is used. By starting the process with the path directly and setting
UseShellExecute = true, Windows will use the registered handler (typically explorer.exe) but in a way that prevents zombie processes.
338-343:✅ Verification successful
Verify this approach works for all folder paths
The new method of opening folders by passing the path directly to
Process.StartwithUseShellExecute = truemight behave differently for certain path formats, network paths, or special Windows folders.
🏁 Script executed:
#!/bin/bash # Script to check if there are any tests or usage patterns that might be affected by the changes # Look for tests related to the OpenDirectory method rg -n "OpenDirectory" --type cs # Check if there are any special path handling cases in the codebase rg -n "UNC|\\\\\\\\|\\\\\\\\" --type cs # Check for special folder references that might be affected rg -n "Environment.SpecialFolder|GetFolderPath" --type cs # Look for any explorer.exe process handling rg -n "explorer\\.exe|Process\\.Start.*explorer" --type csLength of output: 24095
Process.Start with UseShellExecute=true correctly opens all physical folders (local and UNC)
The change hands off folder opening to ShellExecute, which transparently supports drive-letter paths, network (UNC) shares, long paths, spaces, etc. Special shell namespaces (Recycle Bin, Control Panel, etc.) remain handled by the existing explicit
Process.Start("explorer", …)calls in:
Plugins/Flow.Launcher.Plugin.Sys/Main.cs:354Flow.Launcher.Infrastructure/FileExplorerHelper.cs:57No additional verification or code changes are needed here.
…rove path handling
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
317-317: Parameter naming inconsistency with interface.The parameter names have been changed from PascalCase to camelCase, but the interface
IPublicAPI.OpenDirectorystill uses PascalCase parameters:DirectoryPathandFileNameOrFilePath. This creates an inconsistency between the interface and implementation.Either:
- Update the interface to match this implementation, or
- Revert to using PascalCase parameters to maintain consistency with the interface
🧹 Nitpick comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
346-359: Consider adding exception handling.While the custom file manager logic looks good, there's no exception handling around the
Process.Startcalls. If the explorer application doesn't exist or can't be started, this could throw an unhandled exception.- Process.Start(psi); + try + { + Process.Start(psi); + } + catch (Exception ex) + { + LogException(nameof(PublicAPIInstance), $"Failed to open directory with custom explorer: {explorerInfo.Path}", ex); + // Fallback to default explorer as a recovery mechanism + Process.Start(new ProcessStartInfo + { + FileName = targetPath, + UseShellExecute = true + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/PublicAPIInstance.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher/PublicAPIInstance.cs
[warning] 49-52: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 145-174: VSTHRD is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
317-331: Improved path resolution logic.The new implementation properly handles different path scenarios, making the code more robust.
332-344: Good fix for zombie explorer.exe processes.This specialized handling for Windows Explorer addresses the core issue described in the PR. By using
Process.StartwithUseShellExecute = trueand passing only the target path (without explicitly starting explorer.exe), the code now leverages the Windows Shell to open folders, preventing zombie processes.
This comment has been minimized.
This comment has been minimized.
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. If the flagged items are 🤯 false positivesIf items relate to a ...
|
3 participants
Summary
explorer.exeto open a folder path would result in a lingering zombie process even after the File Explorer window is closed.Problem
Previously, we used
ProcessStartInfowithFileName = "explorer.exe"andArguments = "folder path"to open directories. However, when launched this way from a WPF application,explorer.exeremains as a child process and does not exit even after its window is closed.Solution
Instead of launching
explorer.exedirectly, this PR updates the logic to:Process.Start("folder path")withUseShellExecute = truewhen the custom explorer is set toexplorer.exe.explorer.exe.This method ensures that:
explorer.exeprocesses are left behind.Additional Notes
"explorer.exe".