Asynchronous Loading & Initialization Plugin Model to Improve Window Startup Speed #3854
Conversation
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRefactors plugin lifecycle to use concurrent per-state collections, adds per-plugin results-updated registration via a new IResultUpdateRegister, enables incremental DialogJump plugin registration, adds per-plugin translation update, and adjusts startup sequencing and public API getters. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as PluginsLoader
participant PM as PluginManager
participant MV as MainViewModel (IResultUpdateRegister)
participant Intl as Internationalization
participant DJ as DialogJump
Note over Loader,PM: Load phase
Loader->>PM: LoadPlugins() -> populate _allLoadedPlugins
Note over PM,Intl: Per-plugin translation
PM->>Intl: UpdatePluginMetadataTranslation(plugin)
Note over PM,MV: Initialize phase (parallel)
PM->>MV: InitializePluginsAsync(register: IResultUpdateRegister)
MV->>PM: RegisterResultsUpdatedEvent(pluginPair) %% per-plugin event wiring
PM->>DJ: InitializeDialogJumpPlugin(pluginPair) %% incremental dialog registration
Note right of MV: Runtime ResultsUpdated flow
MV->>MV: On ResultsUpdated(pluginPair) -> Clone results -> Enqueue for UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs (1)
710-710: Fix typo in log message- API.LogDebug(ClassName, $"Destory dialog: {hwnd}"); + API.LogDebug(ClassName, $"Destroy dialog: {hwnd}");Flow.Launcher/App.xaml.cs (1)
413-413: Fix spelling error in comment.Pipeline failure indicates "acees" should be "access".
- // So here we need to check it and just return so that we will not acees _mainWindow?.Dispatcher + // So here we need to check it and just return so that we will not access _mainWindow?.Dispatcher
🧹 Nitpick comments (6)
Flow.Launcher.Core/Resource/Internationalization.cs (1)
370-384: Consider refactoring to eliminate code duplicationThe implementation looks correct. However, this method duplicates the logic from
UpdatePluginMetadataTranslations(lines 356-367). Consider refactoring the existing method to use this new one to maintain DRY principles.public static void UpdatePluginMetadataTranslations() { // Update plugin metadata name & description foreach (var p in PluginManager.GetTranslationPlugins()) { - if (p.Plugin is not IPluginI18n pluginI18N) return; - try - { - p.Metadata.Name = pluginI18N.GetTranslatedPluginTitle(); - p.Metadata.Description = pluginI18N.GetTranslatedPluginDescription(); - pluginI18N.OnCultureInfoChanged(CultureInfo.CurrentCulture); - } - catch (Exception e) - { - API.LogException(ClassName, $"Failed for <{p.Metadata.Name}>", e); - } + UpdatePluginMetadataTranslation(p); } }Flow.Launcher/App.xaml.cs (1)
184-185: Minor improvement: Move log level setup comment.The comment positioning could be improved for better readability.
- // Setup log level before any logging is done - Log.SetLogLevel(_settings.LogLevel); + // Setup log level before any logging is done + Log.SetLogLevel(_settings.LogLevel);Flow.Launcher.Core/Plugin/PluginManager.cs (4)
609-617: Consider using AddOrUpdate for atomic operations.While the current implementation is thread-safe, there's a small window between
TryGetValueandTryAddwhere another thread could add the same key. Consider usingAddOrUpdatefor a more atomic operation:-if (_nonGlobalPlugins.TryGetValue(newActionKeyword, out var item)) -{ - _nonGlobalPlugins.TryUpdate(newActionKeyword, plugin, item); -} -else -{ - _nonGlobalPlugins.TryAdd(newActionKeyword, plugin); -} +_nonGlobalPlugins.AddOrUpdate(newActionKeyword, plugin, (key, oldValue) => plugin);
502-502: Fix typo in comment.-// Plugins may have multi-actionkeywords eg. WebSearches. In this scenario it needs to be overriden on the plugin level +// Plugins may have multi-actionkeywords eg. WebSearches. In this scenario it needs to be overridden on the plugin level
361-361: Consider using collection expressions consistently.For consistency with the rest of the file, consider replacing
Array.Empty<PluginPair>()with collection expressions:-return Array.Empty<PluginPair>(); +return [];Also applies to: 372-372, 377-377
887-893: Use discard for unused out parameters.The out parameters in TryRemove operations are not used. Consider using discards for clarity:
-_allPlugins.TryRemove(plugin.ID, out var item); -_globalPlugins.TryRemove(plugin.ID, out var item1); +_allPlugins.TryRemove(plugin.ID, out _); +_globalPlugins.TryRemove(plugin.ID, out _);-_nonGlobalPlugins.Remove(key, out var item2); +_nonGlobalPlugins.TryRemove(key, out _);Also note: Use
TryRemoveinstead ofRemoveforConcurrentDictionary(line 892).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Flow.Launcher.Core/Plugin/IResultUpdateRegister.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(21 hunks)Flow.Launcher.Core/Resource/Internationalization.cs(1 hunks)Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs(4 hunks)Flow.Launcher/App.xaml.cs(3 hunks)Flow.Launcher/MainWindow.xaml.cs(2 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Flow.Launcher.Core/Plugin/IResultUpdateRegister.cs (2)
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Flow.Launcher/MainWindow.xaml.cs (5)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #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: Yusyuriv
PR: #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: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: onesounds
PR: #3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style ItemHotkeyBGStyle that provides background and border styling, containing a TextBlock with style ItemHotkeyStyle that handles the text styling.
Flow.Launcher.Core/Resource/Internationalization.cs (4)
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #3850
File: Flow.Launcher.Core/Resource/Internationalization.cs:0-0
Timestamp: 2025-07-20T07:28:28.055Z
Learning: In Flow Launcher's Internationalization class, when the Flow Launcher language directory or default language file is missing, the only viable approach is to log an error and return early - there are no fallback mechanisms or alternative recovery strategies available due to architectural constraints.
Learnt from: taooceros
PR: #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/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs (2)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Flow.Launcher/App.xaml.cs (9)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #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: #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: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: taooceros
PR: #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: #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: Yusyuriv
PR: #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: #3850
File: Flow.Launcher.Core/Resource/Internationalization.cs:0-0
Timestamp: 2025-07-20T07:28:28.055Z
Learning: In Flow Launcher's Internationalization class, when the Flow Launcher language directory or default language file is missing, the only viable approach is to log an error and return early - there are no fallback mechanisms or alternative recovery strategies available due to architectural constraints.
Learnt from: Jack251970
PR: #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.
Flow.Launcher/ViewModel/MainViewModel.cs (4)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Yusyuriv
PR: #3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the MainViewModel class, the _lastQuery field is initialized in the constructor and is never null.
Learnt from: Jack251970
PR: #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.
Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs (9)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Yusyuriv
PR: #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: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: #3500
File: Flow.Launcher/Storage/TopMostRecord.cs:145-149
Timestamp: 2025-05-01T05:38:25.673Z
Learning: For the MultipleTopMostRecord implementation in Flow.Launcher, sequence order of records in the ConcurrentBag does not need to be preserved, as confirmed by the developer. The unordered nature of ConcurrentBag is acceptable for this implementation.
Learnt from: jjw24
PR: #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: #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: #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: onesounds
PR: #3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style ItemHotkeyBGStyle that provides background and border styling, containing a TextBlock with style ItemHotkeyStyle that handles the text styling.
Learnt from: Jack251970
PR: #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.
Flow.Launcher/PublicAPIInstance.cs (2)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Flow.Launcher.Core/Plugin/PluginManager.cs (9)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: jjw24
PR: #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: Yusyuriv
PR: #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: #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: #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: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
Learnt from: taooceros
PR: #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: #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.
🧬 Code Graph Analysis (1)
Flow.Launcher.Core/Plugin/IResultUpdateRegister.cs (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
RegisterResultsUpdatedEvent(277-321)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/MainWindow.xaml.cs
[warning] 69-69: Spell check warning: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 118-118: Spell check warning: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 376-376: Spell check warning: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 778-778: Spell check warning: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 779-779: Spell check warning: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 782-782: Spell check warning: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 785-785: Spell check warning: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 788-788: Spell check warning: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 804-804: Spell check warning: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 805-805: Spell check warning: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 810-810: Spell check warning: positionreset is not a recognized word. (unrecognized-spelling)
[warning] 928-928: Spell check warning: XRatio is not a recognized word. (unrecognized-spelling)
[warning] 929-929: Spell check warning: YRatio is not a recognized word. (unrecognized-spelling)
[warning] 1143-1143: Spell check warning: clocksb is not a recognized word. (unrecognized-spelling)
[warning] 1144-1144: Spell check warning: clocksb is not a recognized word. (unrecognized-spelling)
[warning] 1145-1145: Spell check warning: iconsb is not a recognized word. (unrecognized-spelling)
[warning] 1146-1146: Spell check warning: iconsb is not a recognized word. (unrecognized-spelling)
[warning] 1151-1151: Spell check warning: clocksb is not a recognized word. (unrecognized-spelling)
[warning] 1152-1152: Spell check warning: iconsb is not a recognized word. (unrecognized-spelling)
[warning] 234-234: Spell check warning: activing is not a recognized word. (unrecognized-spelling)
[warning] 280-280: Spell check warning: gamemode is not a recognized word. (unrecognized-spelling)
[warning] 418-418: Spell check warning: mainwindow is not a recognized word. (unrecognized-spelling)
[warning] 506-506: Spell check warning: Arrowkeys is not a recognized word. (unrecognized-spelling)
[warning] 784-784: Spell check warning: positionreset is not a recognized word. (unrecognized-spelling)
[error] 825-825: Forbidden pattern error: work around matches a forbidden pattern '\bwork[- ]arounds?\b'. (forbidden-pattern)
Flow.Launcher/App.xaml.cs
[warning] 217-217: Spell check warning: Loadertask is not a recognized word. (unrecognized-spelling)
[warning] 210-210: Spell check warning: Loadertask is not a recognized word. (unrecognized-spelling)
[warning] 413-413: Spell check warning: acees is not a recognized word. (unrecognized-spelling)
Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs
[warning] 180-180: Spell check warning: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 185-185: Spell check warning: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 190-190: Spell check warning: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 195-195: Spell check warning: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 336-336: Spell check warning: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 12-12: Spell check warning: NHotkey is not a recognized word. (unrecognized-spelling)
[warning] 113-113: Spell check warning: preinstalled is not a recognized word. (unrecognized-spelling)
[warning] 175-175: Spell check warning: PInvoke is not a recognized word. (unrecognized-spelling)
[warning] 329-329: Spell check warning: Wnd is not a recognized word. (unrecognized-spelling)
[warning] 710-710: Spell check warning: Destory is not a recognized word. (unrecognized-spelling)
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 502-502: Spell check warning: overriden is not a recognized word. (unrecognized-spelling)
[warning] 798-798: Spell check warning: CETYU is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (22)
Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs (3)
68-73: Good use of ConcurrentDictionary for thread safetyThe migration from
DictionarytoConcurrentDictionaryis appropriate for the asynchronous plugin loading model. This ensures thread-safe access when plugins are initialized concurrently.
109-115: LGTM! Simplified initialization approachThe removal of parameters and use of
TryAddfor preinstalled components aligns well with the new plugin initialization model.
130-151: Well-designed plugin registration methodThe new
InitializeDialogJumpPluginmethod properly supports individual plugin registration with appropriate type checking and thread-safe addition to the concurrent collections.Flow.Launcher/MainWindow.xaml.cs (1)
479-479: Correct use of thread-safe plugin access methodThe change from
PluginManager.NonGlobalPluginsproperty toPluginManager.GetNonGlobalPlugins()method ensures thread-safe access to the plugin collection, which is essential with the new asynchronous plugin loading model.Flow.Launcher.Core/Plugin/IResultUpdateRegister.cs (1)
1-12: Well-designed interface for plugin event registrationThe
IResultUpdateRegisterinterface provides a clean abstraction for registering plugin result update events. This design properly separates concerns and enables dependency injection of the registration handler, which aligns perfectly with the asynchronous plugin loading architecture.Flow.Launcher/PublicAPIInstance.cs (1)
254-254: LGTM! Thread-safe plugin access implemented correctly.The change from property access to method call aligns with the asynchronous plugin loading model and thread-safe concurrent collections refactor.
PluginManager.GetAllPlugins()provides a safe snapshot of plugins without exposing the internal concurrent collection.Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs (1)
118-118: LGTM! Consistent with thread-safe plugin management refactor.The change from
PluginManager.AllPluginsproperty toPluginManager.GetAllPlugins()method call maintains the lazy initialization pattern while adapting to the new thread-safe plugin collection infrastructure.Flow.Launcher/App.xaml.cs (5)
191-192: LGTM! Notification system initialization moved appropriately.Moving notification system initialization earlier ensures it's available before any notification API calls, which prevents potential issues with the asynchronous plugin loading.
246-262: LGTM! Asynchronous plugin initialization achieves PR objectives.This refactoring successfully implements the core objective of separating plugin initialization from main window startup. The async task ensures:
- Main window starts immediately without waiting for plugins
- Plugin loading happens in parallel without blocking UI
- Proper logging tracks plugin initialization performance
- Settings are saved after plugin environment updates
The fire-and-forget pattern (
_ = API.StopwatchLogInfoAsync(...)) is appropriate here since the main startup flow shouldn't wait for plugins to complete.
200-201: Fix spelling error in comment.Pipeline failure indicates a spelling issue that should be corrected.
- // Clean up after portability update + // Clean up after portability updateActually, let me check the pipeline failure more carefully - it seems to be about "Loadertask" and "acees" elsewhere. This line looks correct.
224-227: DialogJump initialization verifiedA search through the codebase confirms:
- InitializeDialogJump now seeds the built-in explorers/dialogs into its concurrent dictionaries.
- PluginManager.cs still calls DialogJump.InitializeDialogJumpPlugin(pair) for each plugin.
- SetupDialogJump is invoked both at startup and on settings changes to (un)register the hotkey.
All plugin and built-in dialog jump paths remain registered via the concurrent dictionaries, so existing functionality is preserved. No further changes required.
252-254: Integration Verified: Plugin loading and initialization are correctly wired up.
PluginManager.LoadPlugins(PluginsSettings)returns aList<PluginPair>as expected.PluginManager.InitializePluginsAsync(List<PluginPair>, IResultUpdateRegister)matches the call in App.xaml.cs.MainViewModelimplementsIResultUpdateRegister, so passing_mainVMsatisfies the interface requirement.No further changes needed.
Flow.Launcher/ViewModel/MainViewModel.cs (4)
30-30: LGTM: Interface implementation aligns with the new plugin event registration pattern.The addition of
IResultUpdateRegisterinterface is correctly implemented with the correspondingRegisterResultsUpdatedEventmethod.
277-321: Well-structured refactoring of the event registration method.The changes improve the design by:
- Making plugin registration explicit with the
PluginPairparameter- Adding proper type checking for
IResultUpdatedinterface- Maintaining thread-safety with result cloning
The event handler correctly preserves all the original functionality including token cancellation checks and metadata updates.
440-440: Correct usage of the new snapshot-returning methods.The migration from
PluginManager.NonGlobalPluginsproperty toPluginManager.GetNonGlobalPlugins()method aligns with the thread-safe design where methods return snapshots of the concurrent collections.Also applies to: 1576-1576, 1596-1596
1330-1330: CancelAsync usage is safe with the current target frameworksAll projects target net9.0-(windows), which is ≥ .NET 8.0 and fully supports CancellationTokenSource.CancelAsync(). No further action is required.
Flow.Launcher.Core/Plugin/PluginManager.cs (6)
33-44: Excellent migration to thread-safe concurrent collections.The choice of collection types is well-considered:
ConcurrentDictionaryfor keyed plugin lookupsConcurrentBagfor unordered plugin lists where iteration is the primary operationThis ensures thread-safety for the new asynchronous plugin loading model.
196-209: Clean separation of plugin loading from initialization.The refactored
LoadPluginsmethod now has a single responsibility - loading plugins and returning them. This enables the new asynchronous initialization pattern while maintaining backwards compatibility.
238-313: Well-designed asynchronous plugin initialization with comprehensive error handling.The initialization flow properly handles:
- Parallel plugin initialization for improved startup performance
- Graceful error handling that disables failed plugins while still tracking them
- Proper event registration through the
IResultUpdateRegisterinterface- Translation updates after API instance is available
The decision to add failed plugins to
_allPlugins(line 278) is correct as it allows users to manage/remove problematic plugins through the UI.
522-540: Clean implementation of snapshot-returning methods.The methods correctly return immutable snapshots of the concurrent collections, preventing external modification. The use of collection expressions provides clean and efficient syntax.
798-798: Plugin ID contains an unusual character sequence.The plugin ID
"5043CETYU6A748679OPA02D27D99677A"contains "CETYU" which was flagged by spell check. If this is intentional (which it appears to be as part of a GUID-like identifier), this can be ignored. Otherwise, please verify the correct ID.
374-375: Use internal PluginModified method for consistency.Based on previous learnings, prefer using the internal
PluginModifiedmethod directly within PluginManager:-if (API.PluginModified(plugin.Metadata.ID)) +if (PluginModified(plugin.Metadata.ID))This aligns with the established pattern for better performance and architectural design.
Likely an incorrect or invalid review comment.
|
🥷 Code experts: taooceros Jack251970, taooceros 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:
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: ✨ Comment |
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.Core/Plugin/PluginManager.cs (1)
646-655: Race condition in RemoveActionKeyword persists.The race condition identified in previous reviews still exists. Between TryRemove and the conditional re-add, another thread could register a different plugin for the same keyword.
🧹 Nitpick comments (2)
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
121-125: Consider caching plugin lists for better performance.Each call to
GetAllInitializedPlugins()creates a new list from the concurrent dictionary. For methods that might be called frequently (like external preview operations), consider caching the plugin list or using the concurrent collections directly with proper filtering.Example optimization for one method:
-await Task.WhenAll([.. GetAllInitializedPlugins().Select(plugin => plugin.Plugin switch +await Task.WhenAll([.. _allPlugins.Values.Select(plugin => plugin.Plugin switchAlso applies to: 130-134, 139-143
887-893: Use consistent variable naming for removed items.The out variables have inconsistent names (item, item1, item2). While this doesn't affect functionality, consistent naming improves readability.
-_allPlugins.TryRemove(plugin.ID, out var item); -_globalPlugins.TryRemove(plugin.ID, out var item1); +_allPlugins.TryRemove(plugin.ID, out var _); +_globalPlugins.TryRemove(plugin.ID, out var _); var keysToRemove = _nonGlobalPlugins.Where(p => p.Value.Metadata.ID == plugin.ID).Select(p => p.Key).ToList(); foreach (var key in keysToRemove) { - _nonGlobalPlugins.Remove(key, out var item2); + _nonGlobalPlugins.TryRemove(key, out var _); }Also note: use
TryRemoveinstead ofRemovefor consistency with ConcurrentDictionary API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Core/Plugin/PluginManager.cs(21 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/PublicAPIInstance.cs
- Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Flow.Launcher.Core/Plugin/PluginManager.cs (9)
Learnt from: Jack251970
PR: #3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: #3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Yusyuriv
PR: #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: #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: #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: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
Learnt from: jjw24
PR: #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: taooceros
PR: #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: #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.
🧬 Code Graph Analysis (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (8)
Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs (5)
PluginsSettings(7-106)Plugin(92-99)Plugin(101-105)Plugin(108-134)UpdatePluginSettings(41-90)Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (3)
Save(134-137)List(33-33)List(57-83)Flow.Launcher.Plugin/Interfaces/ISavable.cs (1)
Save(19-19)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (3)
PluginModified(539-539)List(177-177)List(445-445)Flow.Launcher.Core/Resource/Internationalization.cs (4)
List(322-327)Internationalization(17-387)Internationalization(34-37)UpdatePluginMetadataTranslation(370-384)Flow.Launcher.Core/Plugin/PluginsLoader.cs (2)
List(25-56)PluginsLoader(17-166)Flow.Launcher.Core/Plugin/PluginConfig.cs (1)
PluginConfig(12-144)Flow.Launcher.Core/Plugin/QueryBuilder.cs (1)
Query(9-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Report (PR)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Flow.Launcher/App.xaml.cs (1)
216-216: Variable naming corrected.The variable name has been updated to
imageLoaderTaskwith proper camelCase, addressing the previous spelling warning.Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs (1)
11-11: Spell checker false positive (duplicate).Same as in App.xaml.cs, 'iNKORE' is part of the package namespace and should be added to spell check exceptions.
🧹 Nitpick comments (1)
Flow.Launcher/App.xaml.cs (1)
25-25: Consider adding 'iNKORE' to spell check exceptions.The pipeline reports 'NKORE' as unrecognized, but this is part of the
iNKORE.UI.WPF.Modernpackage namespace. Consider adding this to the spell checker's exception list to suppress the false positive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher/App.xaml.cs(3 hunks)Flow.Launcher/MainWindow.xaml.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher/MainWindow.xaml.cs
- Flow.Launcher/PublicAPIInstance.cs
- Flow.Launcher/ViewModel/MainViewModel.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher/App.xaml.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher/App.xaml.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher/App.xaml.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher/App.xaml.cs
🧬 Code graph analysis (2)
Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs (2)
Flow.Launcher/PublicAPIInstance.cs (3)
List(250-250)List(252-253)List(532-532)Flow.Launcher.Core/Plugin/PluginManager.cs (5)
List(559-562)List(564-575)List(577-580)List(587-590)List(628-655)
Flow.Launcher/App.xaml.cs (7)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
SetLogLevel(66-84)Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
ImageLoader(17-411)Flow.Launcher/MainWindow.xaml.cs (3)
MainWindow(37-1498)MainWindow(86-102)InitializeDialogJump(1432-1438)Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs (3)
DialogJump(20-1086)InitializeDialogJump(105-124)SetupDialogJump(149-316)Flow.Launcher/Helper/HotKeyMapper.cs (2)
HotKeyMapper(13-168)Initialize(20-31)Flow.Launcher/PublicAPIInstance.cs (2)
SaveAppAllSettings(107-116)LogInfo(278-279)Flow.Launcher.Core/Plugin/PluginManager.cs (2)
PluginManager(24-1047)PluginManager(168-174)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs
[warning] 11-11: unrecognized-spelling: 'NKORE' is not a recognized word.
Flow.Launcher/App.xaml.cs
[warning] 25-25: unrecognized-spelling: 'NKORE' is not a recognized word.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher/App.xaml.cs (5)
190-191: LGTM! Early log level setup is correct.Setting the log level before any logging operations is the right approach and the comment clearly explains the requirement.
197-198: LGTM! Notification system initialization is properly positioned.Installing the notification system before any notification API calls prevents potential initialization issues.
206-207: LGTM! Portability cleanup is correctly positioned.Performing cleanup after language initialization is appropriate since cleanup operations may require localized messages.
230-234: LGTM! DialogJump initialization sequencing is correct.The initialization is properly positioned after main window creation (to access window handle) and before hotkey mapper initialization. The comments clearly document the sequencing requirements.
252-275: LGTM! Asynchronous plugin initialization correctly implements the PR objectives.The plugin initialization is now decoupled from main window startup, allowing the UI to show immediately while plugins load in the background. Key improvements:
- Fire-and-forget pattern allows main window to start without blocking
- Home page refresh logic (lines 264-267) handles users opening the window during initialization, ensuring complete results once all plugins finish loading
- Maintains proper sequencing: path updates → load plugins → initialize plugins → updates → save
This change successfully addresses the core issue where slow plugins blocked the main window from appearing.
Based on learnings
Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginsViewModel.cs (1)
117-130: LGTM! Property changes correctly support asynchronous plugin initialization.The changes appropriately handle the new plugin lifecycle:
- Changing from
IListtoListaligns with the underlying API return type- Using
App.API.GetAllPlugins()now includes plugins in all states (Initializing, Initialized, Init failed)- The comments clearly explain why all plugin states are needed: to allow uninstalling failed plugins and changing settings for initializing plugins
This is essential for the async plugin loading feature, ensuring users can manage plugins even while initialization is in progress.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
34-34: Consider thread-safe initialization for Settings field.The
Settingsfield could be accessed beforeLoadPluginscompletes in the new asynchronous model, potentially causing a null reference exception. Since other threads may query plugins whileLoadPluginsis still executing, race conditions are possible.Based on learnings
Consider one of these approaches:
- Initialize with a default value:
private static PluginsSettings Settings = new PluginsSettings();- Use
Lazy<PluginsSettings>for thread-safe lazy initialization- Add null checks before accessing
Settingsthroughout the classApply this diff to initialize with a default value:
-private static PluginsSettings Settings; +private static PluginsSettings Settings = new PluginsSettings();
🧹 Nitpick comments (2)
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
289-293: Misleading variable name for failed plugin collection.Failed plugins are added to
_allInitializedPlugins(line 291), but these plugins are NOT successfully initialized. The variable name implies successful initialization, which is semantically incorrect and could confuse future maintainers.Based on past review comments
Consider one of these approaches:
- Rename
_allInitializedPluginsto_allProcessedPluginsor_pluginsAfterInitto indicate they've been through initialization (successfully or not)- Use a separate collection exclusively for successfully initialized plugins
- Add a clarifying comment explaining that this collection includes both successful and failed initializations for UI purposes
The comment at lines 289-290 explains the rationale ("so that we can remove the plugin from Plugin or Store page"), but the variable name doesn't reflect this dual purpose.
670-721: Reduce code duplication in plugin state check methods.The three methods
IsInitializingOrInitFailed,IsInitializing, andIsInitializationFailed(lines 670-721) have substantial duplicate logic checking the same conditions. This duplication increases maintenance burden and the risk of inconsistencies.Consider consolidating the logic:
+private enum PluginInitState { NotLoaded, Initializing, Failed, Succeeded } + +private static PluginInitState GetPluginInitState(string id) +{ + if (!_allLoadedPlugins.ContainsKey(id)) return PluginInitState.NotLoaded; + if (!_allInitializedPlugins.ContainsKey(id)) return PluginInitState.Initializing; + return _initFailedPlugins.ContainsKey(id) ? PluginInitState.Failed : PluginInitState.Succeeded; +} + public static bool IsInitializingOrInitFailed(string id) { - // Id does not exist in loaded plugins - if (!_allLoadedPlugins.ContainsKey(id)) return false; - - // Plugin initialized already - if (_allInitializedPlugins.ContainsKey(id)) - { - // Check if the plugin initialization failed - return _initFailedPlugins.ContainsKey(id); - } - // Plugin is still initializing - else - { - return true; - } + var state = GetPluginInitState(id); + return state == PluginInitState.Initializing || state == PluginInitState.Failed; } public static bool IsInitializing(string id) { - // Id does not exist in loaded plugins - if (!_allLoadedPlugins.ContainsKey(id)) return false; - - // Plugin initialized already - if (_allInitializedPlugins.ContainsKey(id)) - { - return false; - } - // Plugin is still initializing - else - { - return true; - } + return GetPluginInitState(id) == PluginInitState.Initializing; } public static bool IsInitializationFailed(string id) { - // Id does not exist in loaded plugins - if (!_allLoadedPlugins.ContainsKey(id)) return false; - - // Plugin initialized already - if (_allInitializedPlugins.ContainsKey(id)) - { - // Check if the plugin initialization failed - return _initFailedPlugins.ContainsKey(id); - } - // Plugin is still initializing - else - { - return false; - } + return GetPluginInitState(id) == PluginInitState.Failed; }This approach centralizes the state determination logic and makes the public methods clearer and more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Plugin/PluginManager.cs(22 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
🧬 Code graph analysis (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (6)
Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs (5)
PluginsSettings(7-106)Plugin(92-99)Plugin(101-105)Plugin(108-134)UpdatePluginSettings(41-90)Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs (1)
DataLocation(6-45)Flow.Launcher.Core/Resource/Internationalization.cs (4)
Internationalization(16-407)Internationalization(30-33)UpdatePluginMetadataTranslation(380-394)List(332-337)Flow.Launcher.Core/Plugin/PluginConfig.cs (4)
PluginConfig(12-140)List(21-61)List(63-99)PluginMetadata(101-139)Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs (2)
DialogJump(20-1086)InitializeDialogJumpPlugin(126-147)Flow.Launcher.Core/Plugin/QueryBuilder.cs (1)
Query(9-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
Flow.Launcher.Core/Plugin/PluginManager.cs (7)
257-263: RegisterPluginActionKeywords is called before plugin initialization.At line 262,
RegisterPluginActionKeywords(pair)is invoked beforepair.Plugin.InitAsync()(line 267), but the comment states "Register plugin action keywords so that plugins can be queried in results." This order means plugins are queryable before they finish initializing, which could cause issues.However, looking at the query methods (lines 398-419, 466-487), you've added
IsPluginInitializingchecks that show a "still initializing" message. This design appears intentional to allow early registration while handling uninitialized state gracefully.Please confirm this ordering is intentional. If so, consider updating the comment to clarify: "Register plugin action keywords early so plugins can be discovered in queries, even while still initializing. Query methods handle uninitialized state with appropriate messaging."
206-221: Good error handling in plugin loading.The plugin loading logic (lines 206-221) properly uses
TryAddto detect duplicate plugin IDs and logs errors (line 214) while continuing to process remaining plugins. This resilient approach prevents one problematic plugin from blocking the entire loading process.
176-190: Improved error handling for DeletePythonBinding.The addition of try-catch blocks (lines 181-189) around
File.Deleteoperations properly handles potential exceptions (file in use, access denied, etc.) and ensures all plugin directories are processed even if some deletions fail. The debug logging provides useful diagnostics without cluttering production logs.
1014-1030: Clean uninstall logic with proper concurrent dictionary operations.The uninstall implementation (lines 1014-1030) correctly uses
TryRemovewith the discard pattern (out _) to remove plugins from all relevant collections. The sequential removal from multiple dictionaries ensures complete cleanup without race conditions.
323-339: Well-implemented plugin keyword registration.The
RegisterPluginActionKeywordsmethod (lines 323-339) properly uses thread-safe operations:
Distinct()(line 327) prevents duplicate registrations when a plugin has the same keyword multiple timesTryAddoperations (lines 332, 335) ensure atomic insertions- Clear separation of global vs. non-global keywords
559-591: Well-designed plugin collection accessors.The new getter methods (lines 559-591) follow good practices:
- Return copies of internal collections to prevent external modification (e.g.,
[.. _allLoadedPlugins.Values])GetAllInitializedPluginsprovides flexibleincludeFailedparameter for different use cases- Consistent filtering using
PluginModifiedwhere appropriateThese accessors properly encapsulate the internal concurrent collections while providing necessary read access.
826-830: Appropriate use of includeFailed parameter.The comment at line 826 correctly explains why
GetAllInitializedPlugins(includeFailed: true)is used here: to check version conflicts against all plugins, even those that failed initialization. This prevents installing a plugin that would conflict with an existing (but broken) installation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher/App.xaml.cs (1)
216-216: Concurrent image preload with manifest update + variable naming fixed.Running ImageLoader concurrently with manifest update reduces startup wall time, and the imageLoaderTask naming resolves prior spelling warnings.
Please confirm the spelling check now passes in CI.
Also applies to: 223-223
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/App.xaml.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher/App.xaml.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher/App.xaml.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher/App.xaml.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher/App.xaml.cs
🧬 Code graph analysis (1)
Flow.Launcher/App.xaml.cs (8)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
SetLogLevel(66-84)Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
ImageLoader(17-411)Flow.Launcher.Infrastructure/Http/Http.cs (2)
Http(13-234)Http(21-30)Flow.Launcher.Infrastructure/DialogJump/DialogJump.cs (3)
DialogJump(20-1086)InitializeDialogJump(105-124)SetupDialogJump(149-316)Flow.Launcher/Helper/HotKeyMapper.cs (2)
HotKeyMapper(13-168)Initialize(20-31)Flow.Launcher/PublicAPIInstance.cs (2)
SaveAppAllSettings(107-116)LogInfo(278-279)Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (1)
PreStartPluginExecutablePathUpdate(171-217)Flow.Launcher.Core/Plugin/PluginManager.cs (3)
PluginManager(24-1047)PluginManager(168-174)LoadPlugins(200-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher/App.xaml.cs (3)
190-196: Good ordering: log level set and notifications installed before usage.Ensures correct log filtering and avoids early notification initialization issues.
Also applies to: 197-199
206-208: Pre-start portability cleanup moved earlier — sensible.Prevents stale paths before subsequent operations.
230-234: DialogJump initialization order is correct.Init after MainWindow (needs handle) and before hotkey mapper (registers hotkey). Matches new API.
Updated the log level for the plugin constructor cost message from `LogInfo` to `LogDebug` to reduce verbosity in production logs and make this information available primarily during debugging sessions.
Removed the `Score = -100` property from multiple result objects in `PluginManager.cs` to simplify the code and improve clarity. Adjusted the formatting and structure to ensure proper syntax and maintain code consistency. This includes changes to result objects that handle re-querying and exception handling.
Co-authored-by: Jeremy Wu <jeremy24wu@gmail.com>
Co-authored-by: Jeremy Wu <jeremy24wu@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
1023-1027: Minor race condition in non-global plugin cleanup.The pattern at lines 1023-1027 builds a list of keys to remove and then iterates to remove them. Between collecting the keys (line 1023) and removing each key (lines 1024-1027), another thread could modify _nonGlobalPlugins for the same plugin. However, given that this code runs during uninstall with the plugin already marked as modified (which blocks queries), this race is unlikely to cause issues in practice.
If strictness is desired, consider using a lock around lines 1023-1027 to make the collection and removal atomic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginManager.cs(22 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(2 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
- Flow.Launcher/Languages/en.xaml
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-10-16T10:48:30.551Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.551Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
📚 Learning: 2025-10-16T09:29:19.633Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:519-523
Timestamp: 2025-10-16T09:29:19.633Z
Learning: In Flow Launcher's PluginManager.cs QueryDialogJumpForPluginAsync method, when a DialogJump plugin is still initializing, returning null is intentional behavior. This allows the query to fall through to the default Explorer plugin, which serves as a fallback handler. This is different from QueryForPluginAsync and QueryHomeForPluginAsync, which show "still initializing" messages because they don't have fallback mechanisms.
Applied to files:
Flow.Launcher.Core/Plugin/PluginManager.cs
🧬 Code graph analysis (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (8)
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (1)
PluginPair(60-67)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (1)
PluginPair(55-62)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (1)
PluginPair(55-62)Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs (5)
PluginsSettings(7-106)Plugin(92-99)Plugin(101-105)Plugin(108-134)UpdatePluginSettings(41-90)Flow.Launcher/ViewModel/MainViewModel.cs (5)
RegisterResultsUpdatedEvent(280-324)Query(1198-1226)List(1318-1378)Result(1754-1795)Result(1797-1825)Flow.Launcher/PublicAPIInstance.cs (7)
PluginModified(588-588)LogDebug(275-276)LogError(284-285)List(250-250)List(252-253)List(532-532)ActionKeywordAssigned(270-270)Flow.Launcher.Core/Resource/Internationalization.cs (4)
Internationalization(16-407)Internationalization(30-33)UpdatePluginMetadataTranslation(380-394)List(332-337)Flow.Launcher.Plugin/PluginMetadata.cs (1)
PluginMetadata(10-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: gitStream.cm
- GitHub Check: build
- GitHub Check: Check Spelling
🔇 Additional comments (10)
Flow.Launcher.Core/Plugin/PluginManager.cs (10)
28-40: LGTM: Thread-safe collections for async lifecycle.The migration to concurrent collections (_allLoadedPlugins, _allInitializedPlugins, _initFailedPlugins, _globalPlugins, _nonGlobalPlugins, and specialized bags) correctly supports the asynchronous plugin initialization model, preventing race conditions during parallel plugin loading and initialization.
176-190: LGTM: Error handling prevents cascading failures.The try-catch wrapper around
File.Deleteensures that if one directory's binding file deletion fails (e.g., file in use, access denied), the loop continues processing remaining directories. The debug logging provides visibility into failures without disrupting startup.
261-262: Early action keyword registration may allow queries to uninitialized plugins.
RegisterPluginActionKeywordsis called at line 262 beforeInitAsync(lines 266-267), meaning action keywords are registered even if initialization fails. This could allow queries to target plugins that haven't completed initialization, though the query methods do checkIsPluginInitializing.Verify that the query validation in
ValidPluginsForQueryand query methods adequately prevents queries to plugins that registered keywords but failed to initialize. If so, this is safe; otherwise, consider registering keywords only after successful initialization.
323-360: LGTM: Thread-safe keyword and list registration.Both
RegisterPluginActionKeywordsandAddPluginToListsuse appropriate concurrent operations (TryAdd for dictionaries, Add for ConcurrentBag) ensuring thread-safe registration during parallel initialization. TheDistinct()call on line 327 prevents duplicate keyword registrations for plugins with multiple identical action keywords.
398-418: LGTM: Initialization check prevents queries to unready plugins.The initialization check at lines 398-418 correctly returns a user-facing "still initializing" result when a plugin hasn't completed initialization, preventing errors and providing clear feedback. The auto-requery action (line 412) helps refresh results once initialization completes.
Based on learnings
556-587: LGTM: Getter methods provide thread-safe snapshots.All getter methods correctly create snapshots of the concurrent collections:
GetAllLoadedPlugins()andGetAllInitializedPlugins()use collection expressions to create new listsGetNonGlobalPlugins()usesToDictionary()to create a new dictionaryGetTranslationPlugins()filters and creates a new listThis prevents callers from holding references to the internal concurrent collections while supporting safe enumeration.
667-718: LGTM: Initialization status checks provide clear plugin state.The three status-checking methods correctly distinguish between:
IsInitializingOrInitFailed: Plugin exists but isn't ready (either still initializing or failed to initialize)IsInitializing: Plugin exists and is currently initializing (not yet in _allInitializedPlugins)IsInitializationFailed: Plugin completed initialization but failed (in both _allInitializedPlugins and _initFailedPlugins)This provides clear state differentiation for UI and query logic.
724-730: LGTM: Thread-safe action keyword registration check.The
ActionKeywordRegisteredmethod correctly usesContainsKeyon the concurrent dictionary, which is thread-safe and provides an atomic check for action keyword registration.
764-790: LGTM: Action keyword removal is thread-safe.Both global (line 772) and non-global (line 777) action keyword removals use
TryRemovewith discard pattern, which is the correct thread-safe approach for concurrent dictionaries. The metadata updates (lines 781-789) are also appropriate.
210-216: No safeguard needed for plugin.Metadata
Metadata is always initialized by PluginsLoader (and in tests) and never null, so accessing plugin.Metadata.ID inside the existingplugin != nullcheck is safe.Likely an incorrect or invalid review comment.
CHANGES
Since plugin initialization can cost plenty of time, let us start the main window first and then load and initialize the window to improve window startup speed.
If there is one plugin which consumes long time to load or initialize, main window will show UI to notify users and query other plugins for results so that all things in main window will not affected by that initializing plugin.
Fix #2854
TEST
await Task.Delay(30000)inPlugin Managerplugin. Main window toggle and query still work. If users query this plugin, it will tell users that this plugin is still initializing: