Add internal model for plugin management #3572
Conversation
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: onesounds onesounds 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:
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. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an internal model for plugin management to remove the dependency on the external Plugin Manager plugin, thereby preventing Flow Launcher from entering a bad state when that plugin is deleted.
- Refactored the deletion command in PluginViewModel to use an internal uninstall method.
- Updated PluginStoreItemViewModel with new async methods for installing, uninstalling, and updating plugins, and refactored plugin properties.
- Added a new user setting with corresponding UI and language translation keys for automatically restarting Flow Launcher after plugin changes.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/PluginViewModel.cs | Removed PluginManagerActionKeyword and updated delete plugin command to use the internal model. |
| Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs | Refactored plugin data handling and introduced async plugin management methods (install, uninstall, update). |
| Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml | Added a card to toggle the new auto restart setting. |
| Flow.Launcher/Languages/en.xaml | Added new translation keys for plugin management messages and auto restart functionality. |
| Flow.Launcher/Infrastructure/UserSettings/Settings.cs | Introduced a new AutoRestartAfterChanging setting. |
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs:26
- [nitpick] Consider renaming '_newPlugin' to 'currentPlugin' and '_oldPluginPair' to 'installedPluginPair' for clearer understanding.
_newPlugin = plugin;
📝 Walkthrough""" WalkthroughThis update introduces a new static Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Settings/Plugin Store)
participant PluginInstaller
participant API
participant PluginManager
User->>UI (Settings/Plugin Store): Initiates plugin install/update/uninstall
UI (Settings/Plugin Store)->>PluginInstaller: Calls async method (Install/Update/Uninstall)
PluginInstaller->>API: Show confirmation dialog (if needed)
API-->>PluginInstaller: User response
PluginInstaller->>PluginManager: Perform operation (install/update/uninstall)
PluginManager-->>PluginInstaller: Operation result
PluginInstaller->>API: Show result, prompt for restart (if needed)
API-->>User: Displays message and/or triggers restart
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (1)
160-197: Mirror the ‘restart-on-error’ fix in Uninstall flowSame unconditional restart occurs here. Insert an early
returninsidecatch, or capture asuccessflag shared with the final block.
🧹 Nitpick comments (3)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (3)
119-137: Partial download artefact not cleaned when user cancelsWhen the user presses “Cancel” in the progress box the temp zip (possibly half-written) is left on disk, and the next install run deletes only if
deleteFileis true and the filename collides. Consider explicit cleanup on cancellation:if (cts.IsCancellationRequested) { - return; + if (!newPlugin.IsFromLocalInstallPath && File.Exists(filePath)) + File.Delete(filePath); + return; }
199-254: Update flow: stale zip file not deleted after successful update
UpdatePluginAsyncdownloads to%TEMP%but never deletes the zip afterwards (unlike install). This can quickly clutter%TEMP%with large archives.@@ else { await App.API.UpdatePluginAsync(oldPlugin, newPlugin, filePath); + if (!newPlugin.IsFromLocalInstallPath && File.Exists(filePath)) + File.Delete(filePath); }
256-289: Download helper: typo and double-download logic
- Comment line 269 spells “expcetion”.
- When the progress box fails, we silently fall back to a background download without progress or user feedback. Confirm this is desired UX; otherwise surface the error.
Consider surfacing the original exception to the caller instead of a silent retry.
🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 264-264:
prgis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 264-264: Spell check warning:
prgis not a recognized word. (unrecognized-spelling)
📜 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/SettingPages/Views/SettingsPaneGeneral.xaml(1 hunks)Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs(3 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher/ViewModel/PluginViewModel.cs
[warning] 17-17: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs
[warning] 19-19: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 264-264: Spell check warning: prg is not a recognized word. (unrecognized-spelling)
🪛 GitHub Check: Check Spelling
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs
[warning] 19-19:
Ioc is not a recognized word. (unrecognized-spelling)
[warning] 264-264:
prg is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (7)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
179-179: Good addition of the auto-restart settingThe new
AutoRestartAfterChangingproperty with a default value offalseis a good addition that will help control the application's restart behavior after plugin changes.Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)
205-214: Well-structured UI control for auto-restart settingThe toggle switch for the auto-restart setting is properly implemented with:
- Appropriate binding to the Settings.AutoRestartAfterChanging property
- Localized title and tooltip
- Consistent styling with other toggle switches
- Good positioning in the settings hierarchy
This follows the established pattern for settings controls in the application.
Flow.Launcher/ViewModel/PluginViewModel.cs (2)
1-1: Appropriate addition of Task namespaceThe addition of the System.Threading.Tasks namespace is necessary for the async implementation.
173-175: Good conversion to async uninstall methodConverting the plugin uninstall operation to an asynchronous method is a good improvement that:
- Properly uses async/await pattern
- Leverages the centralized uninstall method from PluginStoreItemViewModel
- Supports the PR objective of creating an internal model for plugin management
This change will make the plugin uninstallation process more robust.
Flow.Launcher/Languages/en.xaml (2)
134-135: Well-defined localization strings for auto-restart settingThe localization strings for the auto-restart feature are clear and descriptive, explaining both the purpose of the setting and its effects.
189-204: Comprehensive localization for plugin managementThis set of localization strings provides thorough coverage for the plugin management workflow:
- Error messages for installation/uninstallation/updating failures
- Success messages with restart instructions
- Confirmation prompts with plugin details
- Settings preservation options
- Download progress indicators
These strings will ensure a good user experience with appropriate feedback throughout the plugin management process.
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (1)
98-106: Generated filename may contain invalid path characters
newPlugin.Namecan legally contain:or other characters invalid on Windows, makingPath.Combinethrow.
Sanitise the name (usePath.GetInvalidFileNameChars()) before composing the filename.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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/ViewModel/PluginStoreItemViewModel.cs (1)
32-33: Version parsing remains a potential exception source.This addresses a previous review concern about
new Version(string)throwing exceptions on malformed version strings. The issue persists in the current implementation.Consider using safe version parsing to prevent UI crashes:
- public bool LabelUpdate => LabelInstalled && new Version(_newPlugin.Version) > new Version(_oldPluginPair.Metadata.Version); + public bool LabelUpdate => LabelInstalled && TryParseVersionComparison(_newPlugin.Version, _oldPluginPair.Metadata.Version); + + private static bool TryParseVersionComparison(string newVersion, string oldVersion) + { + return Version.TryParse(newVersion, out var newVer) && + Version.TryParse(oldVersion, out var oldVer) && + newVer > oldVer; + }
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
871-904: Fix spelling issues in parameter name and comment.The download helper method is well-designed with progress reporting and retry logic, but has spelling issues flagged by static analysis.
Apply this diff to fix the spelling issues:
- internal static async Task DownloadFileAsync(string prgBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true) + internal static async Task DownloadFileAsync(string progressBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true) { if (deleteFile && File.Exists(filePath)) File.Delete(filePath); if (showProgress) { var exceptionHappened = false; - await API.ShowProgressBoxAsync(prgBoxTitle, + await API.ShowProgressBoxAsync(progressBoxTitle, async (reportProgress) => { if (reportProgress == null) { - // when reportProgress is null, it means there is expcetion with the progress box + // when reportProgress is null, it means there is exception with the progress box // so we record it with exceptionHappened and return so that progress box will close instantly exceptionHappened = true; return;🧰 Tools
🪛 GitHub Check: Check Spelling
[warning] 879-879:
prgis not a recognized word. (unrecognized-spelling)🪛 GitHub Actions: Check Spelling
[warning] 879-883:
prgis not a recognized word. (unrecognized-spelling)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginManager.cs(4 hunks)Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs(4 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/ViewModel/PluginViewModel.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (5)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-478)Plugins/Flow.Launcher.Plugin.PluginsManager/Utilities.cs (1)
UserPlugin(62-80)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (8)
GetTranslation(136-136)MessageBoxResult(366-366)InstallPlugin(511-511)LogException(276-276)ShowMsgError(85-85)RestartApp(34-34)ShowMsg(114-114)ShowMsg(123-123)Flow.Launcher.Core/Resource/Internationalization.cs (1)
GetTranslation(247-259)Flow.Launcher.Plugin/PluginMetadata.cs (1)
PluginMetadata(10-163)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 879-879:
prg is not a recognized word. (unrecognized-spelling)
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 39-53: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 117-133: Reloadable is not a recognized word. (unrecognized-spelling)
[warning] 179-183: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 181-185: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 182-186: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 184-188: metadatas is not a recognized word. (unrecognized-spelling)
[warning] 879-883: prg is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher.Core/Plugin/PluginManager.cs (4)
28-28: LGTM: FlowSettings integration looks good.The static field retrieval of Settings via IoC is appropriate here since PluginManager is a static class and the settings are used for runtime behavior control.
553-624: Excellent async plugin installation with comprehensive user experience.The method provides:
- User confirmation before proceeding
- Progress reporting during download with cancellation support
- Proper error handling with early return on failure (prevents unwanted restart)
- Conditional restart based on user settings
- Clean file management for temporary downloads
626-664: Well-implemented uninstall flow with proper user choices.The method correctly:
- Confirms the uninstall action with the user
- Provides choice for keeping/removing plugin settings
- Handles errors gracefully without restarting on failure
- Follows consistent UX pattern with other plugin operations
666-722: Robust update implementation following established patterns.The update flow properly handles the two-plugin scenario (old and new) and maintains consistency with install/uninstall operations regarding user confirmation, error handling, and restart behavior.
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (2)
12-18: Excellent refactoring improves code clarity and separation of concerns.The split into
_newPluginand_oldPluginPairfields makes the code more readable and eliminates repeated lookups. The constructor properly initializes both fields for consistent state.
63-79: Async conversion successfully integrates with new plugin management workflow.The method correctly dispatches to the appropriate async PluginManager methods based on the action. The developer has chosen to keep the default case minimal as noted in past reviews.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Flow.Launcher.Core/Plugin/PluginInstaller.cs (3)
40-42: Consider using a more deterministic filename for version-less plugins.Using
Guid.NewGuid()for plugins without version information could make troubleshooting difficult and creates unnecessary randomness.Consider using a timestamp or plugin ID instead:
- var downloadFilename = string.IsNullOrEmpty(newPlugin.Version) - ? $"{newPlugin.Name}-{Guid.NewGuid()}.zip" - : $"{newPlugin.Name}-{newPlugin.Version}.zip"; + var downloadFilename = string.IsNullOrEmpty(newPlugin.Version) + ? $"{newPlugin.Name}-{DateTime.Now:yyyyMMddHHmmss}.zip" + : $"{newPlugin.Name}-{newPlugin.Version}.zip";
275-276: Consider making hardcoded values configurable.The hardcoded GitHub URL values reduce flexibility for supporting other trusted sources in the future.
Consider extracting these to configuration:
- var acceptedHost = "github.com"; - var acceptedSource = "https://github.com"; + var trustedSources = new[] { "github.com" }; + var acceptedSource = "https://github.com";
18-287: Consider dependency injection and testability improvements.The static class design makes unit testing difficult and creates tight coupling to UI components.
For better testability and maintainability, consider:
- Converting to an instance class with interface abstraction
- Injecting UI dialog services instead of calling API.ShowMsgBox directly
- Extracting file operations to a separate service
This would enable proper unit testing and better separation of concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Core/Plugin/PluginInstaller.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginStoreViewModel.cs(2 hunks)Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs(3 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs(2 hunks)Flow.Launcher/ViewModel/SelectBrowserViewModel.cs(0 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher/ViewModel/SelectBrowserViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher/ViewModel/PluginViewModel.cs
- Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginStoreViewModel.cs
- Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Flow.Launcher.Core/Plugin/PluginInstaller.cs (4)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/Helper/PluginInstallationHelper.cs:251-252
Timestamp: 2025-06-29T08:31:07.794Z
Learning: In Flow Launcher's PluginInstallationHelper, when the progress box encounters an exception during download (indicated by reportProgress becoming null), the retry logic intentionally downloads without a progress handler to avoid repeating the same progress reporting failure. This prevents cascading exceptions in the progress reporting mechanism.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
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.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginInstaller.cs
[warning] 237-237:
prg is not a recognized word. (unrecognized-spelling)
[warning] 26-26:
Ioc is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher.Core/Plugin/PluginInstaller.cs (3)
24-26: Good defensive programming with lazy API initialization.The comment and implementation correctly handle the IoC service resolution to avoid circular dependencies in static constructors.
240-256: Excellent exception handling for progress reporting.The retry logic correctly handles progress box exceptions by downloading without progress reporting, which aligns with the established pattern in the codebase.
269-274: Add bounds checking for URL parsing.Array access without bounds checking could cause IndexOutOfRangeException.
Add proper validation:
var pieces = url.Split('/'); if (pieces.Length < 4) return false; - var author = pieces[3]; + var author = pieces.Length > 3 ? pieces[3] : string.Empty; + + if (string.IsNullOrEmpty(author)) + return false;Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Just found a couple minor issues:
- After uninstalling pm, store no longer fetches plugin until clicking on refresh.
- Using pm install/uninstall does not automatically restart flow even though the option is set
- Store install/uninstall same plugin without restart shows error message error, should say already installed/uninstalled
- pm install/uninstall same plugin without restart says correct message but another message also pops up to say it's successfully installed
- pm uninstall same plugin without restart says correct message but string interpolation is not working
…in until clicking on refresh.
… shows error message error, should say already installed/uninstalled
…ys correct message but another message also pops up to say it's successfully installed
Fixed. This is because originally, Flow rely on pm to initialize the manifest. So we need to explicitly call update when app is loading.
You need to change the option in pm setting panel instead of Flow general setting page.
Fixed. Now Flow will show msg when encountering those issues.
Fixed. Now Flow will show msg when encountering those issues.
I cannot reproduce that. Please give the specific message. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (1)
33-33: Address version parsing exception risk.The
new Version()constructor can throwArgumentExceptionfor malformed or empty version strings, potentially crashing the UI.- public bool LabelUpdate => LabelInstalled && new Version(_newPlugin.Version) > new Version(_oldPluginPair.Metadata.Version); + public bool LabelUpdate => LabelInstalled && + Version.TryParse(_newPlugin.Version, out var newVer) && + Version.TryParse(_oldPluginPair.Metadata.Version, out var oldVer) && + newVer > oldVer;
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginInstaller.cs (1)
250-250: Fix parameter name spelling.The parameter name
prgBoxTitleshould be corrected for better readability.- private static async Task DownloadFileAsync(string prgBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true) + private static async Task DownloadFileAsync(string progressBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true)And update the usage on line 258:
- await API.ShowProgressBoxAsync(prgBoxTitle, + await API.ShowProgressBoxAsync(progressBoxTitle,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Core/Plugin/PluginInstaller.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(4 hunks)Flow.Launcher/App.xaml.cs(1 hunks)Flow.Launcher/Languages/en.xaml(3 hunks)Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/Languages/en.xaml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Flow.Launcher/App.xaml.cs (4)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Flow.Launcher.Core/Plugin/PluginManager.cs (7)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/Helper/PluginInstallationHelper.cs:251-252
Timestamp: 2025-06-29T08:31:07.794Z
Learning: In Flow Launcher's PluginInstallationHelper, when the progress box encounters an exception during download (indicated by reportProgress becoming null), the retry logic intentionally downloads without a progress handler to avoid repeating the same progress reporting failure. This prevents cascading exceptions in the progress reporting mechanism.
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.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3081
File: Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs:136-145
Timestamp: 2025-02-22T23:51:54.010Z
Learning: In Flow.Launcher JsonRPCV2 plugins, the `reload_data` method is optional for plugins to implement. When the method is not found (RemoteMethodNotFoundException), it should be silently caught without logging as this is an expected scenario.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.
Flow.Launcher.Core/Plugin/PluginInstaller.cs (5)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/Helper/PluginInstallationHelper.cs:251-252
Timestamp: 2025-06-29T08:31:07.794Z
Learning: In Flow Launcher's PluginInstallationHelper, when the progress box encounters an exception during download (indicated by reportProgress becoming null), the retry logic intentionally downloads without a progress handler to avoid repeating the same progress reporting failure. This prevents cascading exceptions in the progress reporting mechanism.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
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.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3081
File: Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs:136-145
Timestamp: 2025-02-22T23:51:54.010Z
Learning: In Flow.Launcher JsonRPCV2 plugins, the `reload_data` method is optional for plugins to implement. When the method is not found (RemoteMethodNotFoundException), it should be silently caught without logging as this is an expected scenario.
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (9)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/Helper/PluginInstallationHelper.cs:251-252
Timestamp: 2025-06-29T08:31:07.794Z
Learning: In Flow Launcher's PluginInstallationHelper, when the progress box encounters an exception during download (indicated by reportProgress becoming null), the retry logic intentionally downloads without a progress handler to avoid repeating the same progress reporting failure. This prevents cascading exceptions in the progress reporting mechanism.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3081
File: Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs:136-145
Timestamp: 2025-02-22T23:51:54.010Z
Learning: In Flow.Launcher JsonRPCV2 plugins, the `reload_data` method is optional for plugins to implement. When the method is not found (RemoteMethodNotFoundException), it should be silently caught without logging as this is an expected scenario.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3561
File: Flow.Launcher/ViewModel/SelectBrowserViewModel.cs:53-58
Timestamp: 2025-05-21T10:37:07.696Z
Learning: When implementing collection item removal operations in Flow Launcher, always handle index boundaries carefully. After removing an item from a collection, ensure the updated index remains within valid bounds (>= 0 and < collection.Count) to prevent IndexOutOfRangeExceptions, especially when decrementing indexes.
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.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3279
File: Flow.Launcher/Helper/WallpaperPathRetrieval.cs:44-46
Timestamp: 2025-02-28T07:47:24.148Z
Learning: In Flow.Launcher's WallpaperPathRetrieval class, using a `using` statement with MemoryStream when loading images with BitmapImage does not work properly, even when using BitmapCacheOption.OnLoad. The stream needs to remain open while the bitmap is in use.
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginInstaller.cs
[warning] 258-258:
prg is not a recognized word. (unrecognized-spelling)
[warning] 26-26:
Ioc is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
Flow.Launcher/App.xaml.cs (1)
211-212: LGTM! Good timing for manifest update.Adding the plugin manifest update before plugin initialization ensures plugins have access to current manifest data during their initialization process. The placement and async implementation are correct.
Flow.Launcher.Core/Plugin/PluginManager.cs (3)
545-546: Excellent refactoring to user-friendly error handling.Converting from exception-based to return value + user message error handling improves the user experience significantly. The early return pattern prevents unnecessary operations when installation fails.
565-598: Consistent error handling pattern with clear user messages.The method now returns
falseand displays user-friendly messages instead of throwing exceptions. This approach provides better UX and aligns well with the newPluginInstallerarchitecture.
648-653: Good alignment with the new error handling pattern.The uninstall method now follows the same user-friendly approach of showing messages and returning early instead of throwing exceptions.
Flow.Launcher.Core/Plugin/PluginInstaller.cs (3)
18-27: Well-designed centralized plugin management class.The static class design with dependency injection via IoC provides a clean separation of concerns and centralizes plugin lifecycle operations effectively.
28-104: Comprehensive installation flow with proper user interaction.The method handles user confirmation, download progress, error handling, and respects the auto-restart setting. The cancellation token usage and file cleanup logic are implemented correctly.
285-307: Source validation logic is appropriate.The URL parsing and GitHub validation logic provides reasonable security checks for plugin sources while maintaining flexibility for known repositories.
Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs (2)
12-18: Good refactoring to separate concerns.The separation of
_newPluginand_oldPluginPairimproves clarity and makes the intent of each property more explicit.
63-79: Excellent integration with the new PluginInstaller architecture.The async conversion and delegation to
PluginInstallermethods centralizes plugin management logic and provides consistent user experience across the application.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an internal model for plugin management to avoid dependency on the external Plugin Manager and adds the ability to install plugins from a local zip file in the Plugin Store page. Key changes include:
- Refactoring of plugin installation, uninstallation, and updating logic across multiple components.
- Updates to API methods and language resources to support success/failure handling.
- Addition of new UI components and settings for managing plugins and app restart behavior.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.PluginsManager/Utilities.cs | Simplified extraction of plugin.json entry from the zip archive. |
| Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs | Updated control flow for plugin updates and fixed spelling in comments. |
| Flow.Launcher/ViewModel/PluginViewModel.cs | Changed the delete plugin method to async and updated its logic. |
| Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs | Refactored plugin property usage to distinguish between new and existing plugin data. |
| SettingPages/ViewModels/SettingsPanePluginStoreViewModel.cs | Added an async command to support local plugin installation via file dialog. |
| Flow.Launcher/PublicAPIInstance.cs, IPublicAPI.cs | Adjusted API signatures to return success/failure booleans. |
| Flow.Launcher/Core/Plugin/PluginManager.cs & PluginInstaller.cs | Modified plugin installation/uninstallation logic to use boolean returns and improved error handling. |
| Languages/en.xaml | Added new translation keys for the plugin management experience. |
| App.xaml.cs | Updated the app initialization to include plugin manifest updates. |
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/PluginViewModel.cs:173
- [nitpick] Changing the delete plugin action to an asynchronous method is appropriate; ensure that any UI elements interacting with this command properly handle the asynchronous delay so that the user experience remains responsive.
private async Task OpenDeletePluginWindowAsync()
|
|
…ys correct message but another message also pops up to say it's successfully installed. It should only one pop up notification to say that the plugin already installed/uninstalled
…uninstalled from 'Fail to install ' to ' is already installed'. Do the same for uninstalling notification title.
Fixed (I have misunderstood your meaning previously...) I have changed the API functions with return values to indicate the installation/uninstallation states so that pm can know if it needs to show message.
Will fixed it in another PR. #3791
Changed for both store and pm.
One option is for Flow internal store system, and another is for pm plugin. The former is in the general section of setting window, while the latter is in the setting panel of pm plugin. 
I think this is fixed since it is the same as 4. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Flow.Launcher.Core/Plugin/PluginInstaller.cs (4)
22-26: Consider thread safety for static field initialization.The lazy initialization pattern for the API field could potentially have thread safety issues in a multi-threaded environment, though this is likely acceptable for a UI application context.
For improved thread safety, consider using
Lazy<T>:- private static IPublicAPI api = null; - private static IPublicAPI API => api ??= Ioc.Default.GetRequiredService<IPublicAPI>(); + private static readonly Lazy<IPublicAPI> api = new(() => Ioc.Default.GetRequiredService<IPublicAPI>()); + private static IPublicAPI API => api.Value;
182-182: Fix typo in comment.There's a typo in the comment.
- return; // don not restart on failure + return; // do not restart on failure
259-259: Improve parameter naming clarity.The parameter name
prgBoxTitleis unclear and flagged by spell check.- private static async Task DownloadFileAsync(string prgBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true) + private static async Task DownloadFileAsync(string progressBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts, bool deleteFile = true, bool showProgress = true)And update the usage:
- await API.ShowProgressBoxAsync(prgBoxTitle, + await API.ShowProgressBoxAsync(progressBoxTitle,
294-316: Consider making source validation more configurable.The
InstallSourceKnownmethod has hardcoded GitHub-specific logic that could be made more flexible for future extensibility.Consider extracting the trusted source configuration:
+ private static readonly string[] TrustedHosts = { "github.com" }; + private static readonly string[] TrustedSources = { "https://github.com" }; private static bool InstallSourceKnown(string url) { if (string.IsNullOrEmpty(url)) return false; var pieces = url.Split('/'); if (pieces.Length < 4) return false; var author = pieces[3]; - var acceptedHost = "github.com"; - var acceptedSource = "https://github.com"; - var constructedUrlPart = string.Format("{0}/{1}/", acceptedSource, author); - if (!Uri.TryCreate(url, UriKind.Absolute, out var uri) || uri.Host != acceptedHost) + if (!Uri.TryCreate(url, UriKind.Absolute, out var uri) || !TrustedHosts.Contains(uri.Host)) return false; + var matchingSource = TrustedSources.FirstOrDefault(source => url.StartsWith(source)); + if (matchingSource == null) + return false; + + var constructedUrlPart = string.Format("{0}/{1}/", matchingSource, author); return API.GetAllPlugins().Any(x => !string.IsNullOrEmpty(x.Metadata.Website) && x.Metadata.Website.StartsWith(constructedUrlPart) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Core/Plugin/PluginInstaller.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(4 hunks)Flow.Launcher/Languages/en.xaml(3 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/en.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Plugins/Flow.Launcher.Plugin.PluginsManager/Languages/en.xaml
- Flow.Launcher/Languages/en.xaml
- Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs
- Flow.Launcher.Core/Plugin/PluginManager.cs
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher.Core/Plugin/PluginInstaller.cs (5)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/Helper/PluginInstallationHelper.cs:251-252
Timestamp: 2025-06-29T08:31:07.794Z
Learning: In Flow Launcher's PluginInstallationHelper, when the progress box encounters an exception during download (indicated by reportProgress becoming null), the retry logic intentionally downloads without a progress handler to avoid repeating the same progress reporting failure. This prevents cascading exceptions in the progress reporting mechanism.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
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.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3081
File: Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs:136-145
Timestamp: 2025-02-22T23:51:54.010Z
Learning: In Flow.Launcher JsonRPCV2 plugins, the `reload_data` method is optional for plugins to implement. When the method is not found (RemoteMethodNotFoundException), it should be silently caught without logging as this is an expected scenario.
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginInstaller.cs
[warning] 267-267:
prg is not a recognized word. (unrecognized-spelling)
[warning] 26-26:
Ioc is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
Flow.Launcher.Core/Plugin/PluginInstaller.cs (7)
47-49: Good approach for handling missing version information.The GUID generation for filename when version is empty prevents filename conflicts and ensures unique temporary files.
114-122: Appropriate error handling for ZIP validation.The try-catch block properly handles deserialization failures, malformed JSON, and missing plugin.json files, providing clear error messages to users.
137-147: Effective unknown source warning implementation.The conditional warning system allows users to be informed about potentially untrusted sources while maintaining flexibility through the settings toggle.
270-286: Complex but necessary progress handling logic.The null check for
reportProgressand retry logic correctly handles progress reporting failures, as documented in the retrieved learnings. This prevents cascading exceptions in the progress reporting mechanism.
28-107: Well-structured installation flow with comprehensive error handling.The installation method properly handles user confirmation, download progress, validation, cleanup, and restart logic. The error handling ensures users receive appropriate feedback while preventing system instability.
150-198: Robust uninstallation process with user choice preservation.The uninstallation method appropriately prompts users for confirmation and settings removal preference, with proper error handling and restart logic consistent with the installation flow.
200-257: Consistent update mechanism aligning with install/uninstall patterns.The update method follows the same user interaction and error handling patterns as installation and uninstallation, ensuring a cohesive user experience across all plugin management operations.
|
Auto restart of store & PM is set to false by default already. I have no idea how to distinguish them and could you please do me a favor? |
|
And I think they are different clearly since one of them is store, and another is plugin? |
Add internal model for plugin management
Let us do not rely
Plugin Managerplugin for plugin management in Flow Launcher. Flow Launcher will get into a very bad state if this plugin is deleted (plugin store page will lose ability to install/uninstall/update plugins).Resolve #3555, #3236.
Flow supports to select local zip file to install in Plugin Store page.
API functions related to plugin management have return value to check if installation / uninstallation / update is successful.
Test