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. |
📝 WalkthroughWalkthroughThis change refactors plugin management and initialization to use thread-safe concurrent collections, introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant PluginManager
participant MainViewModel
participant IResultUpdateRegister
App->>PluginManager: Prepare environment
App->>PluginManager: LoadPlugins(settings)
PluginManager-->>App: List<PluginPair>
App->>PluginManager: InitializePluginsAsync(IResultUpdateRegister)
PluginManager->>MainViewModel: RegisterResultsUpdatedEvent(plugin)
MainViewModel->>PluginPair: Subscribe to ResultsUpdated event
sequenceDiagram
participant PluginPair
participant MainViewModel
PluginPair->>MainViewModel: ResultsUpdated event
MainViewModel->>MainViewModel: Validate query and cancellation
MainViewModel->>MainViewModel: Clone results, set badge icons, update metadata
MainViewModel->>MainViewModel: Enqueue results for UI update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
How do we indicate/notify that plugin/plugins are still loading and no results will be returned yet? We need to avoid the confusion like e.g. flow is broken because I don't get any bookmarks in the results. |
I do not think we need to do that because it is unlikely that all plugins installed by users will take a long time to initialize. There should be some plugins which are loaded and initialized very fast |
This comment has been minimized.
This comment has been minimized.
|
Remind me again do we clearly log when plugin finished loading? If we don't notify then I think the log should make it easy to see which plugin finished and which is still loading. In case with issues like we experienced in the roll out of favicon loading in Bookmarks plugin. |
Let me check. Flow will log when one plugin is initialized in |
This comment has been minimized.
This comment has been minimized.
And I find plugin constructor log is only for Debug log level, so if users set log level to Info, nothing will be logged. While plugin initialization has both debug and info log level, so it will always disappear in log files. I will add info log message for constructor to resolve this. (Done in Add info log message for plugin constructors) |
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
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, the plugin section in setting window will just display other plugins and main window will query other plugins for results so that all things in main window will not affected by that bad plugin.
TEST
await Task.Delay(100000)inPlugin Managerplugin and main window toggle and query still work.