Improve File Manager Configuration Error Handling #3561
Conversation
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970 Jack251970, onesounds have 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:
Activity based on git-commit:
Knowledge based on git-blame:
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""" WalkthroughThis update modifies the default configuration for the "Files" file manager in user settings, enhances error handling and user feedback when opening directories with custom file managers, adds new UI elements and validation in the file manager selection window, and introduces new English localization strings related to file manager configuration and errors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SelectFileManagerWindow
participant PublicAPIInstance
participant OS
User->>SelectFileManagerWindow: Clicks "Done" after choosing file manager
SelectFileManagerWindow->>SelectFileManagerWindow: Validate file manager path
alt Invalid path
SelectFileManagerWindow->>User: Show warning dialog
User->>SelectFileManagerWindow: Confirm or cancel
alt User cancels
SelectFileManagerWindow-->>User: Cancel save
else User confirms
SelectFileManagerWindow->>PublicAPIInstance: Save and continue
end
else Valid path
SelectFileManagerWindow->>PublicAPIInstance: Save and continue
end
User->>PublicAPIInstance: Request to open directory
PublicAPIInstance->>OS: Attempt to launch file manager
alt File manager not found
PublicAPIInstance->>User: Show "file manager not found" error
else Other error
PublicAPIInstance->>User: Show generic error with details
end
Assessment against linked issues
Suggested labels
Suggested reviewers
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 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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 (
|
📝 WalkthroughWalkthroughThis update modifies the default configuration for the "Files" file explorer, adds new localized UI strings and error messages related to file manager selection, and introduces improved validation and error handling when users select or invoke a custom file manager. The UI for selecting a file manager is enhanced with a new tips button and clearer separation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SelectFileManagerWindow
participant System
User->>SelectFileManagerWindow: Clicks "Done"
SelectFileManagerWindow->>SelectFileManagerWindow: IsFileManagerValid(path)
alt Path invalid
SelectFileManagerWindow->>User: Show warning dialog (Yes/No)
alt User selects No
SelectFileManagerWindow-->>User: Cancel save
else User selects Yes
SelectFileManagerWindow->>System: Proceed to save
end
else Path valid
SelectFileManagerWindow->>System: Save file manager selection
end
sequenceDiagram
participant User
participant PublicAPIInstance
participant System
User->>PublicAPIInstance: OpenDirectory(path, file)
PublicAPIInstance->>System: Try to start process
alt Success
System-->>User: Opens directory or file
else Win32Exception (file not found)
PublicAPIInstance->>User: Show "file manager not found" dialog
else Other exception
PublicAPIInstance->>User: Show "folder open error" dialog
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 ✨ 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: 0
🧹 Nitpick comments (5)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (2)
72-105: Comprehensive file manager path validation implementation.The validation logic is thorough, checking for multiple valid path scenarios:
- The built-in Explorer
- Full paths to existing executables
- Commands available in the system PATH
While the implementation works well, consider adding a simple cache for PATH lookups to avoid running the
wherecommand repeatedly if the validation is called multiple times for the same path.+ private static readonly Dictionary<string, bool> _pathValidationCache = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase); private bool IsFileManagerValid(string path) { if (string.Equals(path, "explorer", StringComparison.OrdinalIgnoreCase)) return true; if (Path.IsPathRooted(path)) { return File.Exists(path); } + // Check cache first + if (_pathValidationCache.TryGetValue(path, out bool isValid)) + return isValid; try { var process = new Process { StartInfo = new ProcessStartInfo { FileName = "where", Arguments = path, RedirectStandardOutput = true, UseShellExecute = false, CreateNoWindow = true } }; process.Start(); string output = process.StandardOutput.ReadToEnd(); process.WaitForExit(); - return !string.IsNullOrEmpty(output); + isValid = !string.IsNullOrEmpty(output); + _pathValidationCache[path] = isValid; + return isValid; } catch { + _pathValidationCache[path] = false; return false; } }🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 125-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 126-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
167-175: Consider adding file type filtering to the file browser dialog.When browsing for file manager executables, the dialog doesn't limit selection to executable files, which could lead users to select invalid files.
private void btnBrowseFile_Click(object sender, RoutedEventArgs e) { Microsoft.Win32.OpenFileDialog dlg = new Microsoft.Win32.OpenFileDialog(); + dlg.Filter = "Executable files (*.exe)|*.exe|All files (*.*)|*.*"; + dlg.Title = (string)Application.Current.FindResource("fileManager_select_title"); var result = dlg.ShowDialog(); if (result == true) { TextBox path = (TextBox)(((FrameworkElement)sender).Parent as FrameworkElement).FindName("PathTextBox"); path.Text = dlg.FileName; path.Focus(); ((Button)sender).Focus(); } }Flow.Launcher/Languages/en.xaml (3)
369-370: Improve clarity of the “Files” button label and tooltip
The button label and tooltip can be more descriptive and concise. For example, “Do you use Files?” could read “Use the Files file manager?” and the tooltip can remove redundant parentheses.- <system:String x:Key="fileManager_files_btn">Do you use Files?</system:String> - <system:String x:Key="fileManager_files_tips">Depending on the version, Files may use either "Files" or "Files-stable" as its path. (This can vary depending on which version you're using.) For more details, see:</system:String> + <system:String x:Key="fileManager_files_btn">Use the Files file manager?</system:String> + <system:String x:Key="fileManager_files_tips">Depending on your version, the Files file manager may use either "Files" or "Files-stable" as its executable name. For more details, see:</system:String>
378-379: Add sentence separation in path-not-found message
The current message merges two sentences without proper punctuation. It should end the first sentence after{1}.- <system:String x:Key="fileManagerPathNotFound">The path for the file manager '{0}' could not be found: '{1}' Do you want to continue?</system:String> + <system:String x:Key="fileManagerPathNotFound">The path for the file manager '{0}' could not be found: '{1}'. Do you want to continue?</system:String>The accompanying title string
fileManagerPathErrorlooks correct.
470-475: Fix spacing and consistency in File Open Error strings
- Add a space in “Settings > General”.
- Ensure naming and capitalization align with other dialogs.
- <system:String x:Key="fileManagerNotFound">The specified file manager could not be found. Please check the Custom File Manager setting under Settings >General.</system:String> + <system:String x:Key="fileManagerNotFound">The specified file manager could not be found. Please check the Custom File Manager setting under Settings > General.</system:String>The titles
fileManagerNotFoundTitle("File Manager Error"),errorTitle("Error"), and thefolderOpenErrormessage are otherwise clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(2 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/SelectFileManagerWindow.xaml(2 hunks)Flow.Launcher/SelectFileManagerWindow.xaml.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
🪛 GitHub Actions: Check Spelling
Flow.Launcher/PublicAPIInstance.cs
[warning] 45-46: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 49-52: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 145-74: VSTHRD is not a recognized word. (unrecognized-spelling)
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 125-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 126-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-44: hyperlink is not a recognized word. (unrecognized-spelling)
Flow.Launcher/SelectFileManagerWindow.xaml
[warning] 125-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 126-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-44: hyperlink is not a recognized word. (unrecognized-spelling)
🪛 GitHub Check: Check Spelling
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 136-136:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 126-126:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 125-125:
hyperlink 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 (7)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
229-231: Updated default configuration for Files file manager.The path for the "Files" file manager has been updated from "Files" to "Files-Stable" and the directory argument has been simplified. This aligns with changes in the Files app mentioned in the PR objectives.
Flow.Launcher/PublicAPIInstance.cs (1)
319-375: Improved error handling with user-friendly messages.The OpenDirectory method now properly handles different failure scenarios when opening directories or files using custom file managers, providing clear and localized error messages to the user.
The implementation correctly:
- Distinguishes between file manager not found errors and other exceptions
- Uses localized error messages for better user experience
- Shows appropriate message boxes with relevant information
Flow.Launcher/SelectFileManagerWindow.xaml (3)
76-82: Added helpful tips button for file manager configuration.This addition improves user experience by providing easy access to important information about file manager configuration.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 125-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 126-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
83-87: Improved UI organization with visual separators.The added separator and adjusted margins create better visual distinction between different sections of the UI, making it more intuitive to use.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 125-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 126-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
123-123: Standardized separator styling.Updated the fill color to use the standard SeparatorForeground resource for consistency across the UI.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 125-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 126-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-44:
hyperlinkis not a recognized word. (unrecognized-spelling)Flow.Launcher/SelectFileManagerWindow.xaml.cs (2)
51-65: Added validation for file manager path with user warning.This important validation check alerts users when they've configured an invalid file manager path, while still allowing them to proceed if they choose to. This directly addresses the PR objective of enhancing feedback when a file manager path is invalid.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 125-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 126-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
107-149: Added informative tips dialog with external resource link.The implementation creates a well-designed dialog with a clickable hyperlink to external documentation, which helps users understand the "Files" file manager variants mentioned in the PR objectives.
The ContentDialog provides a modern UI experience, and the hyperlink implementation correctly uses the system browser to open the URL.
🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 136-136:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 126-126:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 125-125:
hyperlinkis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 125-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 126-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
117-159: Consider reusing the hyperlink navigation logic.The hyperlink navigation code in this method duplicates the functionality you already implemented in the
Hyperlink_RequestNavigatemethod. Consider reusing that method to reduce duplication.- hyperlink.RequestNavigate += (s, args) => - { - Process.Start(new ProcessStartInfo - { - FileName = args.Uri.AbsoluteUri, - UseShellExecute = true - }); - args.Handled = true; - }; + hyperlink.RequestNavigate += Hyperlink_RequestNavigate;🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 146-146:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-136:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 135-135:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 131-131:
hyperlinkis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 131-32:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 135-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 146-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/Languages/en.xaml(2 hunks)Flow.Launcher/SelectFileManagerWindow.xaml(2 hunks)Flow.Launcher/SelectFileManagerWindow.xaml.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/SelectFileManagerWindow.xaml
- Flow.Launcher/Languages/en.xaml
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
🪛 GitHub Check: Check Spelling
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 146-146:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-136:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 135-135:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 131-131:
hyperlink is not a recognized word. (unrecognized-spelling)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 131-32: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 135-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 146-44: hyperlink is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (4)
49-57: Well-implemented hyperlink navigation handler.The
Hyperlink_RequestNavigatemethod correctly handles URL navigation usingProcessStartInfowithUseShellExecute=true. This is the proper way to open URLs in the default browser.🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 131-32:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 135-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 146-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
61-75: Good file manager validation with user-friendly feedback.This validation logic nicely implements the PR objective of improving error handling and user feedback when a file manager path is invalid. The use of localized strings for the dialog is a good practice for internationalization.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 131-32:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 135-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 146-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
82-115: Thorough path validation implementation.The
IsFileManagerValidmethod effectively handles all scenarios:
- Case-insensitive check for "explorer"
- Verification of absolute file paths
- Commands available in the system PATH
The error handling for the "where" command execution is appropriate.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 131-32:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 135-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 146-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
1-15: Appropriate namespace additions.The added namespaces are necessary for the new functionality:
System.Diagnostics: For Process-related operationsSystem.IO: For file/path operationsSystem.Windows.Documents: For hyperlink functionalityModernWpf.Controls: For modern WPF dialog controls
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (2)
61-75: The validation logic looks good, but the message could be clearer.The code correctly validates the file manager path and shows a warning if it's invalid. However, the Yes/No message might not clearly communicate what each option means to the user.
Consider modifying the message to explicitly state what happens with each choice, for example:
var result = App.API.ShowMsgBox( - string.Format((string)Application.Current.FindResource("fileManagerPathNotFound"), - CustomExplorer.Name, CustomExplorer.Path), + string.Format((string)Application.Current.FindResource("fileManagerPathNotFound") + + "\n\nYes = Save anyway\nNo = Cancel and return to edit", + CustomExplorer.Name, CustomExplorer.Path), (string)Application.Current.FindResource("fileManagerPathError"), MessageBoxButton.YesNo, MessageBoxImage.Warning);🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 131-32:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 135-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 146-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
82-115: Comprehensive validation but consider adding a timeout for the process execution.The validation method is well-implemented with checks for Windows Explorer, rooted paths, and commands in PATH. However, the process execution doesn't have a timeout, which could cause the UI to become unresponsive if the
wherecommand takes too long to complete.try { var process = new Process { StartInfo = new ProcessStartInfo { FileName = "where", Arguments = path, RedirectStandardOutput = true, UseShellExecute = false, CreateNoWindow = true } }; process.Start(); + + // Add timeout to prevent UI hanging + bool exited = process.WaitForExit(3000); // 3 second timeout + if (!exited) + { + process.Kill(); + return false; + } + string output = process.StandardOutput.ReadToEnd(); - process.WaitForExit(); return !string.IsNullOrEmpty(output); }Also consider handling paths with arguments (e.g., "notepad.exe C:\file.txt") by extracting just the executable part for validation.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 131-32:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 135-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-22:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 146-44:
hyperlinkis not a recognized word. (unrecognized-spelling)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/MessageBoxEx.xaml.cs(0 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/SelectFileManagerWindow.xaml.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher/MessageBoxEx.xaml.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/PublicAPIInstance.cs
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
🧬 Code Graph Analysis (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (3)
Flow.Launcher/PublicAPIInstance.cs (1)
MessageBoxResult(454-457)Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1)
ProcessStartInfo(190-301)Flow.Launcher/SelectBrowserWindow.xaml.cs (1)
btnDone_Click(42-47)
🪛 GitHub Check: Check Spelling
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 146-146:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-136:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 135-135:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 131-131:
hyperlink is not a recognized word. (unrecognized-spelling)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 131-32: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 135-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-22: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 146-44: hyperlink is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (2)
117-146: Refactor btnTips_Click to reuse existing Hyperlink_RequestNavigate.The click-handler in btnTips_Click duplicates the same Process.Start logic found in Hyperlink_RequestNavigate. Extract the hard-coded URL into a resource or constant and wire up the existing Hyperlink_RequestNavigate method instead of inlining the handler.
private async void btnTips_Click(object sender, RoutedEventArgs e) { var tipText = (string)Application.Current.Resources["fileManager_files_tips"]; - var url = "https://files.community/docs/contributing/updates"; + // Externalize URL for easier maintenance + var url = (string)Application.Current.Resources["fileManager_files_url"] + ?? "https://files.community/docs/contributing/updates"; var textBlock = new TextBlock { FontSize = 14, TextWrapping = TextWrapping.Wrap, Margin = new Thickness(0, 0, 0, 0) }; textBlock.Inlines.Add(tipText); var hyperlink = new Hyperlink { NavigateUri = new Uri(url) }; hyperlink.Inlines.Add(url); - hyperlink.RequestNavigate += (s, args) => - { - Process.Start(new ProcessStartInfo - { - FileName = args.Uri.AbsoluteUri, - UseShellExecute = true - }); - args.Handled = true; - }; + // Reuse the existing navigation handler + hyperlink.RequestNavigate += Hyperlink_RequestNavigate; textBlock.Inlines.Add(hyperlink);🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 137-137:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-136:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 132-132:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 117-117:
VSTHRDis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 117-174:
VSTHRDis not a recognized word. (unrecognized-spelling)
[warning] 132-126:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 136-122:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 137-122:
hyperlinkis not a recognized word. (unrecognized-spelling)
[warning] 147-144:
hyperlinkis not a recognized word. (unrecognized-spelling)
49-57: 🛠️ Refactor suggestionAdd exception handling to the URL navigation.
The
Hyperlink_RequestNavigatemethod needs exception handling to prevent crashes in case of invalid URLs or network/permission issues.private void Hyperlink_RequestNavigate(object sender, System.Windows.Navigation.RequestNavigateEventArgs e) { + try + { Process.Start(new ProcessStartInfo { FileName = e.Uri.AbsoluteUri, UseShellExecute = true }); + } + catch (Exception ex) + { + App.API.ShowMsgBox( + string.Format("Failed to open URL: {0}\nError: {1}", e.Uri.AbsoluteUri, ex.Message), + "Navigation Error", + MessageBoxButton.OK, + MessageBoxImage.Error); + } e.Handled = true; }
🧹 Nitpick comments (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
117-118: Consider explaining the SuppressMessage justification.The current justification of "" for the
VSTHRD100suppression should be replaced with an actual explanation, such as "Event handler must be async void to use await with ContentDialog".🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 117-117:
VSTHRDis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 117-174:
VSTHRDis not a recognized word. (unrecognized-spelling)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/SelectFileManagerWindow.xaml(2 hunks)Flow.Launcher/SelectFileManagerWindow.xaml.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/SelectFileManagerWindow.xaml
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
🪛 GitHub Check: Check Spelling
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 147-147:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 137-137:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-136:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 132-132:
hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 117-117:
VSTHRD is not a recognized word. (unrecognized-spelling)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 117-174: VSTHRD is not a recognized word. (unrecognized-spelling)
[warning] 132-126: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 136-122: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 137-122: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 147-144: hyperlink is not a recognized word. (unrecognized-spelling)
🔇 Additional comments (2)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (2)
61-75: LGTM! Good improvement to validate file manager paths.The validation logic with user-friendly warning messages is a significant improvement that addresses the PR objectives to enhance error handling when configuring file managers.
82-115: Good implementation of path validation with multiple strategies.The
IsFileManagerValidmethod is well-structured with a comprehensive approach:
- Accepts 'explorer' as a valid system command
- Verifies existence of full file paths
- Uses the 'where' command to check if the command exists in PATH
This covers all common scenarios for file manager configuration.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Flow.Launcher/SelectBrowserWindow.xaml.cs (1)
32-43: Consider extracting file dialog path setting logic to a helper method.The path retrieval logic is complex and appears in multiple places. Consider extracting it to a helper method or moving it to the ViewModel.
This is the same suggestion as made for the SelectFileManagerWindow.xaml.cs file.
🧹 Nitpick comments (1)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
39-50: Consider extracting file dialog path setting logic to a helper method.The path retrieval logic is complex and appears in multiple places. Consider extracting it to a helper method or moving it to the ViewModel.
private void btnBrowseFile_Click(object sender, RoutedEventArgs e) { var dlg = new Microsoft.Win32.OpenFileDialog(); var result = dlg.ShowDialog(); if (result == true) { - var path = (TextBox)(((FrameworkElement)sender).Parent as FrameworkElement).FindName("PathTextBox"); + var path = FindPathTextBox(sender); path.Text = dlg.FileName; path.Focus(); ((Button)sender).Focus(); } } + +private TextBox FindPathTextBox(object sender) +{ + return (TextBox)(((FrameworkElement)sender).Parent as FrameworkElement).FindName("PathTextBox"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Flow.Launcher/App.xaml.cs(1 hunks)Flow.Launcher/SelectBrowserWindow.xaml(2 hunks)Flow.Launcher/SelectBrowserWindow.xaml.cs(2 hunks)Flow.Launcher/SelectFileManagerWindow.xaml(3 hunks)Flow.Launcher/SelectFileManagerWindow.xaml.cs(2 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs(1 hunks)Flow.Launcher/ViewModel/SelectBrowserViewModel.cs(1 hunks)Flow.Launcher/ViewModel/SelectFileManagerViewModel.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/SelectFileManagerWindow.xaml
🧰 Additional context used
🧬 Code Graph Analysis (2)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (2)
Flow.Launcher/SelectFileManagerWindow.xaml.cs (2)
SelectFileManagerWindow(9-51)SelectFileManagerWindow(13-18)Flow.Launcher/SelectBrowserWindow.xaml.cs (2)
SelectBrowserWindow(8-44)SelectBrowserWindow(12-17)
Flow.Launcher/SelectBrowserWindow.xaml.cs (3)
Flow.Launcher/ViewModel/SelectBrowserViewModel.cs (3)
SelectBrowserViewModel(9-58)SelectBrowserViewModel(29-34)SaveSettings(36-41)Flow.Launcher/ViewModel/SelectFileManagerViewModel.cs (1)
SaveSettings(47-68)Flow.Launcher/SelectFileManagerWindow.xaml.cs (1)
btnBrowseFile_Click(39-50)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/App.xaml.cs
[warning] 63-63: Spell check warning: WMP is not a recognized word. (unrecognized-spelling)
[warning] 106-106: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 117-117: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 119-119: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 174-174: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 199-199: Spell check warning: Loadertask is not a recognized word. (unrecognized-spelling)
[warning] 225-225: Spell check warning: VSTHRD is not a recognized word. (unrecognized-spelling)
Flow.Launcher/SelectFileManagerWindow.xaml.cs
[warning] 13-13: Spell check warning: Popu is not a recognized word. (unrecognized-spelling)
Flow.Launcher/ViewModel/SelectFileManagerViewModel.cs
[warning] 120-120: Spell check warning: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 124-124: Spell check warning: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 125-125: Spell check warning: hyperlink is not a recognized word. (unrecognized-spelling)
[warning] 131-131: Spell check warning: hyperlink is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (20)
Flow.Launcher/App.xaml.cs (1)
101-104: Good architectural addition for view model registrationThe addition of
SelectBrowserViewModelandSelectFileManagerViewModelas transient services is a good pattern. The transient scope is appropriate since these models contain settings that may change, requiring new instances on subsequent requests.Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (2)
338-339: Correctly updated constructor call to match dependency injection changesThis change aligns with the updated
SelectFileManagerWindowconstructor which now obtains its view model via dependency injection instead of directly accepting aSettingsparameter.
345-346: Correctly updated constructor call to match dependency injection changesThis change aligns with the updated
SelectBrowserWindowconstructor which now obtains its view model via dependency injection instead of directly accepting aSettingsparameter.Flow.Launcher/SelectBrowserWindow.xaml (4)
9-9: Good addition of view model namespaceAdding the view model namespace is necessary for design-time data binding.
12-12: Improved design-time support with DataContextSetting the design-time data context to the view model enhances the design experience and helps validate bindings.
101-102: Good MVVM pattern implementation for Add buttonChanging from event handler to command binding improves separation of concerns and follows MVVM best practices.
105-106: Good MVVM pattern implementation for Delete buttonChanging from event handler to command binding improves separation of concerns and follows MVVM best practices.
Flow.Launcher/ViewModel/SelectBrowserViewModel.cs (2)
1-42: Well-structured view model with good separation of concernsThe view model follows MVVM best practices with:
- Clear property definitions for UI binding
- Proper initialization of collections from settings
- Encapsulation of settings management logic
- Use of observable collections for UI updates
43-52: Clear implementation of Add commandThe Add command follows good practices by creating a new instance with a default name and updating the selection index.
Flow.Launcher/SelectFileManagerWindow.xaml.cs (4)
11-11: Appropriate use of the ViewModel pattern.The code has been updated to use a ViewModel, which is a good practice for separating UI logic from presentation.
13-17: Good use of dependency injection.The window now uses dependency injection to obtain the ViewModel instead of directly managing settings, which promotes better separation of concerns.
🧰 Tools
🪛 GitHub Actions: Check Spelling
[warning] 13-13: Spell check warning:
Popuis not a recognized word. (unrecognized-spelling)
25-30: Improved window closing logic.The window now only closes if settings are successfully saved, which is an improvement over unconditional closing. This ensures that invalid configurations can be addressed before the window closes.
33-37: Better hyperlink navigation.Using
App.API.OpenUrlinstead of directly callingProcess.Startis a good improvement as it centralizes URL handling logic.Flow.Launcher/SelectBrowserWindow.xaml.cs (3)
10-10: Good use of the ViewModel pattern.The code has been updated to use a ViewModel, which is a good practice for separating UI logic from presentation.
12-17: Good use of dependency injection.The window now uses dependency injection to obtain the ViewModel instead of directly managing settings, which promotes better separation of concerns.
26-29: Improved window closing logic.The window now only closes if settings are successfully saved, which is an improvement over unconditional closing. This ensures that invalid configurations can be addressed before the window closes.
Flow.Launcher/ViewModel/SelectFileManagerViewModel.cs (4)
17-39: Well-structured ViewModel implementation.The ViewModel follows proper MVVM patterns with observable collections and property change notifications. The CustomExplorer property correctly retrieves the currently selected item from the collection.
47-68: Good validation in SaveSettings method.The method properly validates the file manager path before saving and gives users a chance to acknowledge or reject invalid paths with a warning message.
70-103: Comprehensive path validation.The IsFileManagerValid method handles multiple validation scenarios properly:
- Special case for "explorer"
- Checking if rooted paths exist
- Using "where" command to locate executables in PATH
This is a robust approach to validation.
146-154: Add command implementation looks good.The Add command correctly adds a new profile and updates the selected index.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
Improves handling and feedback when configuring and opening directories with custom file managers by validating paths, updating defaults, and providing clearer error dialogs.
- Adds validation for custom file manager paths in the settings dialog with user prompts on invalid entries
- Updates default Files manager path and arguments, and includes a “Learn more” link in the UI
- Enhances
OpenDirectoryin the public API with specific exception handling and error messages
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ViewModel/SelectFileManagerViewModel.cs | Validates custom file manager paths and prompts on errors |
| ViewModel/SelectBrowserViewModel.cs | Mirror updates for browser settings dialog |
| SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs | Removes constructor args in dialog launches |
| SelectFileManagerWindow.xaml(.cs) | Migrates to MVVM DI, adds hyperlink and separators |
| SelectBrowserWindow.xaml(.cs) | Migrates to MVVM DI |
| PublicAPIInstance.cs | Wraps OpenDirectory in try/catch with tailored messages |
| MessageBoxEx.xaml.cs | Removes deprecated overload |
| Languages/en.xaml | Adds new translation keys for error titles and messages |
| App.xaml.cs | Registers dialog view models as transient |
| Infrastructure/UserSettings/Settings.cs | Changes default Files path and arguments |
Comments suppressed due to low confidence (3)
Flow.Launcher/ViewModel/SelectFileManagerViewModel.cs:43
- Add unit tests for
SaveSettingsto verify behavior when the custom file manager path is invalid (user Yes/No) and when it is valid.
public bool SaveSettings()
Flow.Launcher/MessageBoxEx.xaml.cs:24
- The overload
Show(string)was removed fromMessageBoxEx, which may break existing calls. Ensure all direct calls are updated or consider restoring it.
public static MessageBoxResult Show(string messageBoxText)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs:230
- The default
DirectoryArgumentwas changed to"%d", removing the-selectflag and potentially breaking folder selection for the default Files manager. Restore-select "%d"if intended.
DirectoryArgument = "\"%d\"",
This comment has been minimized.
This comment has been minimized.
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. 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 ...
|
There was a problem hiding this comment.
Pull Request Overview
This pull request improves the error handling around file manager configuration by providing more informative messages and updating default settings, while also refactoring several UI dialogs to use dependency injection and command bindings.
- More informative error messages are now shown when a custom file manager is not found.
- The default file manager executable path and its arguments have been updated.
- The file manager and browser selection windows have been refactored to use dependency injection and RelayCommands.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/SelectFileManagerViewModel.cs | Added validation and error prompting for invalid file manager paths. |
| Flow.Launcher/ViewModel/SelectBrowserViewModel.cs | Maintains consistency with the file manager view model refactoring. |
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs | Updated window creation to rely on IoC instead of passing Settings directly. |
| Flow.Launcher/SelectFileManagerWindow.xaml.cs | Removed redundant code; switched to dependency injected view model and command bindings. |
| Flow.Launcher/SelectFileManagerWindow.xaml | Updated DataContext and command bindings for cleaner XAML integration. |
| Flow.Launcher/SelectBrowserWindow.xaml.cs | Similar dependency injection and command binding updates for the browser selector. |
| Flow.Launcher/SelectBrowserWindow.xaml | Updated DataContext and command bindings. |
| Flow.Launcher/PublicAPIInstance.cs | Added try/catch logic for handling file manager launch errors with improved error messages. |
| Flow.Launcher/MessageBoxEx.xaml.cs | Removed unused overload for showing message boxes. |
| Flow.Launcher/Languages/en.xaml | Added new translation keys for enhanced error messages and updated the file manager setting texts. |
| Flow.Launcher/App.xaml.cs | Registered the new view models as transient in the dependency injection container. |
| Flow.Launcher/Infrastructure/UserSettings/Settings.cs | Updated default file manager settings to new executable path and argument format. |
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/SelectFileManagerViewModel.cs:77
- Consider using a using-statement (or a using-declaration) for the Process instance to ensure resources are disposed properly after use.
var process = new Process {
3 participants
Improve File Manager Configuration Error Handling
Summary
Details
FilestoFiles-stable. The default setting has been adjusted accordingly.Future
Test
A warning popup should appear when attempting to register a file manager with a non-existent path.
An error message should be shown when trying to open a folder using a file manager that was registered with a non-existent path.