New API Functions Plugin Cache Storage & Use API Functions for Program Plugin & Remove Reflection with Interfaces #3410
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. |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces new API functions to support plugin cache storage using a binary format and updates the Program plugin to use these functions. Key changes include:
- The addition of LoadCacheBinaryStorageAsync/SaveCacheBinaryStorageAsync and SavePluginCaches methods to the Public API.
- Refactoring of the Main and ProgramSetting classes to use the new cache API, including changes to asynchronous indexing methods.
- Updates to related interfaces and storage classes to accommodate binary cache storage via MemoryPack.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs | Made context and settings fields readonly and simplified the constructor signature. |
| Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs |
Updated indexing calls to use asynchronous methods. |
| Plugins/Flow.Launcher.Plugin.Program/Main.cs | Refactored cache loading and saving to use the API, replaced array types with Lists, and updated logging. |
| Flow.Launcher/PublicAPIInstance.cs | Introduced new methods for cache management using a concurrent dictionary of binary storage instances. |
| Flow.Launcher/Interfaces/* | Updated ISavable and IPublicAPI interfaces to include binary cache storage API functions. |
| Flow.Launcher.Infrastructure/Storage/* | Added PluginBinaryStorage and updated BinaryStorage to support new caching operations. |
| Flow.Launcher.Infrastructure/Image/ImageLoader.cs | Adjusted image cache handling by clearing cached data after load. |
| Flow.Launcher.Core/Plugin/* | Updated PluginManager and JSON-RPC API to invoke the new cache saving methods. |
Comments suppressed due to low confidence (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs:239
- [nitpick] There is an inconsistent use of the 'context' field and the static 'Context' property; consider unifying their usage for clearer code consistency.
_win32s = await context.API.LoadCacheBinaryStorageAsync(Win32CacheName, pluginCachePath, new List<Win32>());
📝 WalkthroughWalkthroughThe changes enhance plugin cache management and storage mechanisms by introducing new public methods and interfaces for saving and removing caches. Several components now support asynchronous operations and improved thread safety. Major updates include adding methods like Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JsonRPCAPI
participant PublicAPIInstance
participant PluginBinaryStorage
Client->>JsonRPCAPI: SavePluginCaches()
JsonRPCAPI->>PublicAPIInstance: SavePluginCaches()
PublicAPIInstance->>PluginBinaryStorage: Iterate & Save (sync/async)
PluginBinaryStorage-->>PublicAPIInstance: Confirm Save
sequenceDiagram
participant Main
participant Win32Indexer
participant UWPIndexer
participant UI
Main->>Win32Indexer: IndexWin32ProgramsAsync()
Main->>UWPIndexer: IndexUwpProgramsAsync()
Win32Indexer-->>Main: Return Win32 List
UWPIndexer-->>Main: Return UWP List
Main->>UI: Update Display with Programs
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
40-40: Consider adding a comment explaining the purpose of clearing data.The call to
_storage.ClearData()occurs immediately after loading the data into theusagevariable. While this is functionally correct because the data has already been retrieved, the purpose of clearing the data at this point isn't immediately obvious to future maintainers.var usage = await LoadStorageToConcurrentDictionaryAsync(); +// Clear storage data after loading it into memory to prevent duplicate entries _storage.ClearData();Flow.Launcher.Core/Plugin/PluginManager.cs (1)
591-597: Avoid reflection when removing cached data
Reflection call toRemovePluginCachecan introduce runtime overhead or break if the method signature changes. If possible, callAPI.RemovePluginCache(plugin.PluginCacheDirectoryPath)directly, as it’s already declared inIPublicAPI.Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
347-379: Clarify usage and ensure valid MemoryPack types
The new binary cache methods add valuable functionality. Consider providing usage examples or clarifying error handling when serialized objects are not MemoryPack-compatible. This helps plugin developers integrate safely.Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (3)
33-40: Check file naming for collisions
ConstructingFilePathwithfilename + .cacheis straightforward. If multiple plugins could share a name, consider unique naming or a check to prevent collisions.
83-101: Add robust I/O error handling
WhileSaveAsync()andSave()simplify persistence, simultaneous writes or restricted file permissions could cause failures. Consider adding exception handling or retry logic to minimize data loss.
103-105: Align bothSaveAsyncoverloads
The overload acceptingdatais handy but can bypass the sharedDatafield. Ensure that downstream consumers remain consistent about which version ofSaveAsyncthey call to prevent data drift.Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (1)
112-112: Constructor parameter simplification.
Removing unused parameters (Win32[],UWPApp[]) from the constructor streamlines the class design. Validate that all references to the old parameters have been refactored or removed.Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
270-274: Clearing then re-populating_win32s.
UsingAddRangeor assignment to a newly created list could simplify code and slightly improve performance for large datasets.
282-287: Refilling_uwps.
Mirroring the Win32 approach, consider switching to_uwps.AddRange(uwps)or assigning a new list to improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(2 hunks)Flow.Launcher.Infrastructure/Image/ImageLoader.cs(1 hunks)Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs(3 hunks)Flow.Launcher.Infrastructure/Storage/PluginBinaryStorage.cs(1 hunks)Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs(0 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/ISavable.cs(2 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(7 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs
🧰 Additional context used
🧬 Code Definitions (4)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
SavePluginCaches(351-351)Flow.Launcher/PublicAPIInstance.cs (1)
SavePluginCaches(355-362)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (1)
ClearData(98-101)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (1)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (2)
ValueTask(117-172)ValueTask(281-310)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
SavePluginCaches(189-192)Flow.Launcher/PublicAPIInstance.cs (2)
SavePluginCaches(355-362)T(221-228)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (35)
Flow.Launcher.Infrastructure/Storage/PluginBinaryStorage.cs (1)
5-14: Implementation aligns with the core cache storage API functions mentioned in the PR description.The
PluginBinaryStorage<T>class provides a clean implementation that extendsBinaryStorage<T>for plugin-specific cache storage. It properly validates the directory path and constructs the file path using the provided cache name.Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
188-192: Added method implements the core plugin cache function described in the PR.The
SavePluginCaches()method follows the established pattern of the class by delegating to the corresponding method in the_apiinstance. This implementation provides plugins with access to save their caches through the public API.Flow.Launcher.Plugin/Interfaces/ISavable.cs (1)
9-9: Documentation reference added for the new cache storage API function.The addition of the reference to
IPublicAPI.SaveCacheBinaryStorageAsync{T}(string, string)in the documentation appropriately reflects the new cache storage functionality and provides valuable information to developers implementing theISavableinterface.Flow.Launcher.Core/Plugin/PluginManager.cs (1)
69-69: Ensure separate handling of any cache-saving exceptions
CallingAPI.SavePluginCaches()along with plugin settings is a good addition. Consider whether any exceptions fromSavePluginCaches()need to be caught and logged or retried separately to prevent partial saves.Flow.Launcher/PublicAPIInstance.cs (1)
337-380: Check thread-safety when removing or saving caches
Employing aConcurrentDictionaryhelps, but confirm that removing plugin caches (e.g., viaRemovePluginCache) does not interfere if another task is concurrently loading or saving the same cache. Additional synchronization or design patterns may be needed.Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (6)
7-7: Adopting nullable context
Enabling#nullableis a good step towards safer code. No concerns here.
20-21: Consider concurrency for theDatafield
protected T? Datamight become a contention point if multiple method calls manipulate this field concurrently. Ensure that multi-threaded usage is properly accounted for.
24-27: Validate assignment of init-only properties
The init-only propertiesFilePathandDirectoryPathhelp maintain immutability. Just confirm they are always assigned valid paths pre-use to avoid null references.
29-29: Ensure derived classes handle parameterless constructor
A parameterless constructor can be convenient for inheritance. Verify that subclasses setFilePathandDirectoryPathto valid locations before reading or writing files.
43-45: Potential stale in-memory data
Short-circuiting whenDatais already set skips re-reading the file. Ensure that is intentional and aligns with desired refresh semantics for updated caches.
51-52: Zero-length file usage
SettingDatatodefaultDataand re-saving may overwrite possibly corrupted data. Verify the plugin won’t lose data if the file was truncated accidentally.Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (1)
320-320: Transition to async method call.
Switching fromMain.IndexUwpProgramsto the asyncMain.IndexUwpProgramsAsyncimproves responsiveness. Consider adding exception handling or logging to address potential runtime issues in this background task.Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (1)
800-800: Async directory change handling.
The move toMain.IndexWin32ProgramsAsyncis beneficial for concurrency. Consider whether a try/catch or explicit error logging is needed if indexing fails partway.Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (1)
21-22: Readonly fields for immutability.
Changingcontextand_settingsto readonly clarifies intent and prevents unintended reassignments.Plugins/Flow.Launcher.Plugin.Program/Main.cs (21)
21-21: Removal of ISavable interface.
Ensure all prior save logic is either unnecessary or has been replaced by the newer cache storage approach.
23-24: New constants for cache names.
UsingWin32CacheNameandUwpCacheNameas constants helps maintain clarity and consistency in cache operations.
26-28: Static lists and plugin settings.
Marking_win32s,_uwps, and_settingsas static could lead to concurrency or shared-state issues if multiple plugin instances are ever created. Verify thread safety or usage assumptions.
185-185: Local variable for cache path.
StoringpluginCachePathin a variable keeps the flow clear and reduces repetition.
187-187: Ensuring the cache directory exists.
Helper.ValidateDirectorypreemptively creates or validates the path, minimizing file I/O errors.
232-233: Migrating old Win32 cache.
Transferring the old cache file to the plugin-specific cache directory aids backward compatibility.
235-236: Migrating old UWP cache.
Similar migration logic ensures the user’s existing UWP cache is preserved under the new structure.
239-240: Asynchronous cache loading.
Retrieving_win32sand_uwpsviaLoadCacheBinaryStorageAsyncis non-blocking. Consider adding error handling for potential corruption.
242-243: Logging preload counts.
Useful for debugging how many items were successfully restored from cache.
267-267: Introducing async Win32 index.
This new async method helps keep the UI thread free while searching. Confirm all callers await or handle its completion.
276-276: Asynchronous Win32 cache save.
Writing updated data to disk in an async manner helps avoid blocking. Consider capturing exceptions for recovery.
280-280: Introducing async UWP index.
Analogous to Win32 indexing, ensures consistency across plugin components.
289-289: Saving updated UWP cache.
Completes the lifecycle for UWP items, ensuring changes persist across restarts.
295-295: Spawning Win32 indexing as a separate task.
Runs in parallel with UWP indexing, making indexing more time-efficient.
297-297: Measuring Win32 indexing performance.
A helpful diagnostic for performance tuning and regressions.
300-300: Running UWP indexing concurrently.
Improves responsiveness, leveraging modern async/await patterns.
302-302: Profiling UWP indexing with Stopwatch.
Provides an easy way to track potential performance issues.
305-305: Awaiting both indexing tasks.
A clean pattern that keeps code non-blocking until both tasks complete.
317-317: Simplified settings panel creation.
InstantiatingProgramSettingwith just the context and settings removes extraneous constructor parameters.
373-373: Non-awaited task for UWP re-index.
Without awaiting, exceptions could be lost. Consider logging or capturing errors.
383-383: Non-awaited task for Win32 re-index.
Same concern as UWP: ensure errors are not silently dropped.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Flow.Launcher/PublicAPIInstance.cs (5)
339-350: Code style improvement for TryRemove.The implementation is correct, but you can make the code slightly more readable by using
out _directly instead ofout var _when discarding the result.- _pluginBinaryStorages.TryRemove(key, out var _); + _pluginBinaryStorages.TryRemove(key, out _);
352-362: Missing documentation for SavePluginCaches.While you've provided a summary comment, consider adding more detailed XML documentation that explains when this method is called and how it works with the new binary storage system.
364-371: Missing documentation for LoadCacheBinaryStorageAsync.This public API method should have XML documentation that explains its parameters, return value, and usage scenarios to help plugin developers use it correctly.
+ /// <summary> + /// Loads the binary storage for the current plugin's cache. + /// </summary> + /// <typeparam name="T">The type of data to load</typeparam> + /// <param name="cacheName">Name of the cache</param> + /// <param name="cacheDirectory">Directory of the cache</param> + /// <param name="defaultData">Default data to use if cache doesn't exist</param> + /// <returns>The loaded data or default data if cache doesn't exist</returns> public async Task<T> LoadCacheBinaryStorageAsync<T>(string cacheName, string cacheDirectory, T defaultData) where T : new()
373-380: Missing documentation for SaveCacheBinaryStorageAsync.This public API method should have XML documentation that explains its parameters and usage scenarios to help plugin developers use it correctly.
+ /// <summary> + /// Saves the binary storage for the current plugin's cache. + /// </summary> + /// <typeparam name="T">The type of data to save</typeparam> + /// <param name="cacheName">Name of the cache</param> + /// <param name="cacheDirectory">Directory of the cache</param> + /// <returns>Task representing the asynchronous save operation</returns> public async Task SaveCacheBinaryStorageAsync<T>(string cacheName, string cacheDirectory) where T : new()
364-380: Consider adding exception handling for cache operations.These public API methods currently have no explicit error handling. Consider adding try-catch blocks to handle potential exceptions that might occur during loading or saving operations, especially for file I/O operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/PublicAPIInstance.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
204-204: Good fix for thread-safety.The change from
RemovetoTryRemoveis appropriate as it safely handles concurrent operations in aConcurrentDictionary. This addresses the previous review comment correctly.
337-338: Good design choice for plugin binary storage.Using a tuple key with
(cacheName, cacheDirectory, Type)provides a flexible and type-safe way to store and retrieve plugin binary storage entries.
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
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: To learn more about /:\ gitStream - Visit our Docs |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
242-243: Consider adding null checks before accessing Count propertyWhile the API likely ensures these lists are never null, it's a good practice to add null checks before accessing the Count property to prevent potential NullReferenceExceptions.
-Log.Info($"|Flow.Launcher.Plugin.Program.Main|Number of preload win32 programs <{_win32s.Count}>"); -Log.Info($"|Flow.Launcher.Plugin.Program.Main|Number of preload uwps <{_uwps.Count}>"); +Log.Info($"|Flow.Launcher.Plugin.Program.Main|Number of preload win32 programs <{_win32s?.Count ?? 0}>"); +Log.Info($"|Flow.Launcher.Plugin.Program.Main|Number of preload uwps <{_uwps?.Count ?? 0}>");Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (9)
8-9: Good practice enabling nullable reference types.
Leveraging#nullable enablehelps catch potential null reference issues. Ensure consistent null checks for fields and method parameters throughout the project to fully benefit from this feature.
21-22: Consider convertingDatato a property.
StoringDataas a protected field prevents intercepting changes or enforcing invariants. Converting it to a protected auto-property allows for safer management and potential logging on set operations.
30-30: Parameterless constructor caution.
With a protected parameterless constructor, derived classes must ensureFilePathandDirectoryPathare properly initialized before usage, or file operations will fail.
34-40: Validate filename parameter for safety.
While setting upFilePath, consider defensive checks for invalid or unexpected file names, and handle scenarios wherefilenamemight be empty or contain path-breaking characters.
52-53: Handle zero-length file more robustly.
Currently, the fallback overwrites the file with default data. Depending on usage, logging or alerts may be warranted before overwriting to avoid silent data loss.
63-64: File not present scenario.
Automatically creating a new file fromdefaultDatais convenient, but verify that this is always desired (e.g., for ephemeral or user-managed caches, an unexpected file creation could be undesirable).
75-76: Null check for deserialized object.
ReturningdefaultDatawhentis null is a good fail-safe. For debugging, consider logging such occurrences to detect repeated null-read scenarios.
90-96: Prevent blocking calls if concurrency is expected.
Save()uses synchronous file writes, which can block threads. If file sizes grow, consider using asynchronous writes consistently to avoid UI or worker thread stalls.
104-104: Unify overloadedSaveAsyncmethods.
Currently, there is an overload that savesDatadirectly and another that accepts aT dataparameter. For clarity, consider a singleSaveAsync(T data = default(T))approach, or rename the overloads to differentiate their intent.Flow.Launcher/PublicAPIInstance.cs (2)
351-362: Ensure plugin data references are removed completely.
RemovePluginCacheeffectively removes entries matching the given directory. Validate that leftover states (e.g., partial files) outside_pluginBinaryStoragesare also cleaned up if needed.
385-392: Manual save approach.
SaveCacheBinaryStorageAsyncensures you can save updated caches on demand. Confirm that the plugin calling this method keeps data in-sync to avoid overwriting changes from other threads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Flow.Launcher.Core/Plugin/PluginManager.cs(2 hunks)Flow.Launcher.Infrastructure/Image/ImageLoader.cs(1 hunks)Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs(3 hunks)Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs(0 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(7 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher.Infrastructure/Image/ImageLoader.cs
- Flow.Launcher.Core/Plugin/PluginManager.cs
🧰 Additional context used
🧬 Code Definitions (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (9)
List(218-277)List(833-854)Win32(301-334)Win32(336-392)Win32(394-432)Win32(434-458)Win32(596-611)Win32(672-727)Task(790-802)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (5)
List(476-510)UWPApp(200-239)UWPApp(387-395)Task(295-323)UWPPackage(41-47)Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (2)
ProgramSetting(19-439)ProgramSetting(112-118)
Flow.Launcher/PublicAPIInstance.cs (2)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
SavePluginCaches(189-192)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
SavePluginCaches(351-351)T(242-242)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (17)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (11)
21-21: Interface removal aligns with new caching approachThe
ISavableinterface has been correctly removed from the class declaration. This change is consistent with the PR objective to use the new API functions for plugin cache storage instead of manually implementing save functionality.
23-24: Good use of constants for cache file namesUsing constants for cache file names improves code maintainability by centralizing naming conventions and making future updates easier.
26-28: Enhanced data encapsulation with List propertiesChanging from arrays to Lists with private setters improves encapsulation and provides more flexibility for collection manipulation. This is a good design choice that aligns with the PR's caching improvements.
185-188: LGTM: Proper cache directory validationThe code correctly uses the plugin's cache directory path and validates it before use.
232-237: LGTM: Appropriate cache file path constructionThe code properly constructs paths for the old and new cache files using the newly defined constants, ensuring consistency.
239-240: Improved cache loading with new API functionsThe implementation now uses the new
LoadCacheBinaryStorageAsyncAPI function to load cached data, which aligns with the PR objectives.
267-277: Improved async implementation for Win32 program indexingThe method has been properly converted to async and now uses the new
SaveCacheBinaryStorageAsyncAPI function. The implementation correctly:
- Clears the list before adding new items
- Saves the cache using the new API
- Updates the last index time
280-291: Improved async implementation for UWP program indexingThe method has been properly converted to async and now uses the new
SaveCacheBinaryStorageAsyncAPI function. The implementation follows the same pattern as the Win32 indexing method, ensuring consistency.
293-306: Enhanced parallel execution with Task-based async patternThe implementation properly uses Task.Run with async methods for concurrent execution of Win32 and UWP program indexing, followed by Task.WhenAll to wait for both tasks to complete.
317-317: Simplified ProgramSetting constructor usageThe method now only passes the required parameters to the ProgramSetting constructor, which is consistent with the changes in the ProgramSetting constructor shown in the relevant code snippets.
373-374: Updated method calls to use async versionsThe code correctly updates the method calls to use the new async versions of the indexing methods, which is consistent with the changes made to those methods.
Also applies to: 383-384
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (3)
25-28: Confirm correct usage of the null-forgiving operator.
These properties rely on being assigned before being used; using= null!;suppresses compiler warnings but requires developer discipline to avoid null pointer exceptions at runtime.
57-58: Success path after deserialization.
AssigningData = d;is straightforward. Consider verifying partial data integrity if the reading process might produce partially corrupted objects that still deserialize.
67-67: Early return maintains clarity.
The finalreturn Data;is clear and consistent. Ensure no further logic is needed after data is loaded or created.Flow.Launcher/PublicAPIInstance.cs (3)
216-217: UsingTryRemoveis safer thanRemove.
This change properly handles potential concurrency race conditions when removing plugin settings. No issues here.
349-350: ConcurrentDictionary for binary storages.
Storing plugin caches in_pluginBinaryStoragesis a thread-safe choice. Confirm that type tuples(cacheName, cacheDirectory, Type)are unique enough to avoid collisions.
376-383: Asynchronous cache loading.
LoadCacheBinaryStorageAsyncis well-architected for asynchronous usage. Confirm correct usage in plugins to avoid unawaited tasks that might lead to partial initializations.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Flow.Launcher.Infrastructure/Storage/PluginBinaryStorage.cs (4)
14-15: Consider making the static fieldapireadonly.The static field
apiis used in a lazy initialization pattern, but it's not declared as readonly. To prevent unintended modifications elsewhere in the code, consider marking it as readonly.- private static IPublicAPI api = null; + private static readonly IPublicAPI api = null;
17-23: Add parameter validation for constructor arguments.The constructor doesn't validate if
cacheNameis null or empty, which could lead to invalid file paths.public PluginBinaryStorage(string cacheName, string cacheDirectory) { + if (string.IsNullOrEmpty(cacheName)) + throw new System.ArgumentException("Cache name cannot be null or empty", nameof(cacheName)); + if (string.IsNullOrEmpty(cacheDirectory)) + throw new System.ArgumentException("Cache directory cannot be null or empty", nameof(cacheDirectory)); + DirectoryPath = cacheDirectory; FilesFolders.ValidateDirectory(DirectoryPath); FilePath = Path.Combine(DirectoryPath, $"{cacheName}{FileSuffix}"); }
25-35: Consider usingoverrideinstead ofnewif possible.Using the
newkeyword to hide base class methods can lead to unexpected behavior when used through base class references. If the baseSavemethod can be made virtual, consider usingoverrideinstead.Also, the exception handling is very broad - it catches all exceptions. Consider being more specific with exception types if possible.
-public new void Save() +public override void Save() // If base method can be made virtual { try { base.Save(); } - catch (System.Exception e) + catch (IOException e) // Or other specific exceptions { API.LogException(ClassName, $"Failed to save plugin caches to path: {FilePath}", e); } }
37-47: Apply the same improvements to SaveAsync as suggested for Save.Similar to the
Savemethod, consider usingoverrideinstead ofnewif possible and catching more specific exceptions.-public new async Task SaveAsync() +public override async Task SaveAsync() // If base method can be made virtual { try { await base.SaveAsync(); } - catch (System.Exception e) + catch (IOException e) // Or other specific exceptions { API.LogException(ClassName, $"Failed to save plugin caches to path: {FilePath}", e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Storage/PluginBinaryStorage.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
There was a problem hiding this comment.
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs:93
- Ensure that the 'NonNull()' extension method is properly defined and tested, as its absence or incorrect implementation could lead to null reference exceptions during cache saving.
await SaveAsync(Data.NonNull());
Plugins/Flow.Launcher.Plugin.Program/Main.cs:270
- [nitpick] Consider adding a comment to explain the use of clearing and repopulating _win32s instead of reassigning it, to clarify that preserving the collection reference is intentional.
_win32s.Clear();
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
298-311: Good implementation of thread-safe async UWP program indexingThe UWP indexing method has been properly converted to an async method with thread safety measures through SemaphoreSlim, addressing the previous concerns mentioned in past review comments about thread safety when modifying the global
_uwpslist.
381-424: Fixed the lock release issue in DisableProgramAsyncThe method now correctly releases locks in all code paths, addressing the previous concern mentioned in past review comments where the lock wasn't released if no matching Win32 program was found. The refactoring to an async method also improves consistency with other async methods in the class.
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
252-262: Consider removing unnecessary locks around logging operationsThe locks acquired at lines 252-254 and released at lines 260-261 are only used for logging operations, which don't modify the collections. This could potentially impact performance.
- await _win32sLock.WaitAsync(); - await _uwpsLock.WaitAsync(); - Log.Info($"|Flow.Launcher.Plugin.Program.Main|Number of preload win32 programs <{_win32s.Count}>"); Log.Info($"|Flow.Launcher.Plugin.Program.Main|Number of preload uwps <{_uwps.Count}>"); - _win32sLock.Release(); - _uwpsLock.Release(); -
395-399: Add error handling for fire-and-forget tasksThe code launches tasks but doesn't handle potential exceptions that could occur during indexing. Consider adding error handling to prevent unhandled exceptions.
_ = Task.Run(() => { - _ = IndexUwpProgramsAsync(); + _ = IndexUwpProgramsAsync().ContinueWith(t => + { + if (t.IsFaulted) + { + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Error indexing UWP programs: {t.Exception}"); + } + }); });Similarly for the Win32 indexing task:
_ = Task.Run(() => { - _ = IndexWin32ProgramsAsync(); + _ = IndexWin32ProgramsAsync().ContinueWith(t => + { + if (t.IsFaulted) + { + Log.Error($"|Flow.Launcher.Plugin.Program.Main|Error indexing Win32 programs: {t.Exception}"); + } + }); });Also applies to: 415-419
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs(3 hunks)Flow.Launcher.Plugin/Interfaces/ISavable.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(4 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher.Plugin/Interfaces/ISavable.cs
- Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs
- Flow.Launcher/PublicAPIInstance.cs
🧰 Additional context used
🧬 Code Definitions (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (4)
List(476-510)UWPApp(200-239)UWPApp(387-395)UWPPackage(41-47)Plugins/Flow.Launcher.Plugin.Program/Settings.cs (1)
Settings(9-127)Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (2)
ProgramSetting(19-439)ProgramSetting(112-118)
🔇 Additional comments (6)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (6)
21-28: Code structure improved with interface removal and better property encapsulationThe removal of the
ISavableinterface aligns with the PR objectives of using the new API functions for plugin cache storage. The introduction of constants for cache names and private setters for collections improves encapsulation and code maintainability.
84-119: Improved thread safety with better lock managementThe implementation properly ensures locks are released in all scenarios by using a finally block, which is a good practice for preventing deadlocks. The code now waits for both locks before processing, ensuring data consistency.
191-242: Good cache migration implementationThe code properly handles the migration of cache files from the old location to the new plugin cache directory, with appropriate error handling for file operations.
244-251: Properly implemented async cache loading with lock protectionThe code correctly uses async/await with SemaphoreSlim for thread-safe cache loading. The implementation of
LoadCacheBinaryStorageAsyncaligns with the PR objectives.
283-296: Good implementation of thread-safe async Win32 program indexingThe method now properly uses async/await with SemaphoreSlim for thread safety. The implementation of
SaveCacheBinaryStorageAsyncaligns with the PR objectives, and the lock is correctly released after operations complete.
313-326: Well-structured parallel program indexing with proper task coordinationThe implementation correctly launches Win32 and UWP indexing in parallel using Task.Run and properly awaits both tasks to complete. This ensures efficient resource utilization while maintaining proper async flow.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
189-260:⚠️ Potential issueCache loading implementation with potential issue
The changes for loading cached data using the new async API methods are good, with proper lock acquisition and release within each operation. However, there's an issue with the unconditional lock releases at lines 261-262.
The lines 261-262 are attempting to release locks that may not have been acquired at that point. These lines should be removed as the locks are already properly released at lines 249 and 254.
- _win32sLock.Release(); - _uwpsLock.Release();
🧹 Nitpick comments (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
359-371: Good implementation of async binary cache loadingThis method properly supports generic type constraints with the
where T : new()clause to ensure the type has a parameterless constructor. The documentation is thorough and includes important information about MemoryPack serialization requirements.There's a minor grammar issue in the remarks section:
- /// BinaryStorage utilizes MemoryPack, which means the object must be MemoryPackSerializable <see href="https://github.com/Cysharp/MemoryPack"/> + /// BinaryStorage utilizes MemoryPack, which means the object must be MemoryPackSerializable <see href="https://github.com/Cysharp/MemoryPack"/>Note: This might appear as a no-change diff because it's a grammar issue rather than a code syntax issue. The verb "utilizes" should agree with "BinaryStorage" (singular).
373-385: Proper implementation of async binary cache savingThe method maintains consistency with
LoadCacheBinaryStorageAsyncand includes clear documentation about its behavior. The remarks about automatic saving of previously loaded caches are particularly useful for plugin developers.There's a minor grammar issue in the remarks section:
- /// BinaryStorage utilize MemoryPack, which means the object must be MemoryPackSerializable <see href="https://github.com/Cysharp/MemoryPack"/> + /// BinaryStorage utilizes MemoryPack, which means the object must be MemoryPackSerializable <see href="https://github.com/Cysharp/MemoryPack"/>Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
402-445: Thread-safe implementation of DisableProgramAsyncThis is a good refactoring that ensures proper lock acquisition and release in all code paths. The method now follows async patterns consistently and properly handles thread safety concerns.
Consider using a more structured approach for the fire-and-forget tasks:
- _ = Task.Run(() => - { - _ = IndexUwpProgramsAsync(); - }); + _ = Task.Run(() => IndexUwpProgramsAsync()) + .ContinueWith(t => + { + if (t.IsFaulted) + Log.Exception("|Flow.Launcher.Plugin.Program.Main|Failed to reindex UWP programs", t.Exception); + }, TaskContinuationOptions.OnlyOnFaulted);Apply similar changes to the Win32 reindexing task as well. This ensures that any exceptions in the fire-and-forget tasks are properly logged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Storage/IRemovable.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Core/Storage/IRemovable.cs
🧰 Additional context used
🧬 Code Definitions (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
Flow.Launcher/PublicAPIInstance.cs (2)
SavePluginCaches(378-385)T(241-248)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
SavePluginCaches(193-196)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (4)
List(476-510)UWPApp(200-239)UWPApp(387-395)UWPPackage(41-47)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (8)
List(218-277)List(833-854)Win32(301-334)Win32(336-392)Win32(394-432)Win32(434-458)Win32(596-611)Win32(672-727)Plugins/Flow.Launcher.Plugin.Program/Settings.cs (1)
Settings(9-127)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (7)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
354-357: Excellent addition for plugin cache managementThis method provides a clean, standardized way to save all plugin caches, complementing the existing
SavePluginSettings()andSaveAppAllSettings()methods. It follows the established pattern for global operations in the API.Plugins/Flow.Launcher.Plugin.Program/Main.cs (6)
21-34: Good improvements to class structure and thread safetyThe addition of constants for cache names, private setters for collections, and SemaphoreSlim locks for thread safety are excellent improvements. The static Context property facilitates access throughout the class.
84-119: Improved thread safety in QueryAsync with proper lock managementThe method now correctly acquires and releases locks within a try-finally block, ensuring proper cleanup even if exceptions occur. This is a significant improvement for thread safety.
284-307: Well-structured async implementation of Win32 program indexingThe method now uses proper asynchronous patterns with appropriate error handling. The use of a try-finally block ensures that the lock is released even if exceptions occur, which is critical for thread safety.
309-332: Well-structured async implementation of UWP program indexingSimilar to the Win32 implementation, this method now follows proper asynchronous patterns with appropriate lock management and error handling.
334-347: Efficient task-based parallel indexing implementationThe method now properly creates tasks for both indexing operations and awaits their completion with Task.WhenAll. The use of ConfigureAwait(false) is appropriate in this context to avoid potential deadlocks.
357-359: Changed ProgramSetting constructor signatureThe constructor no longer receives _win32s and _uwps parameters, presumably now accessing these static fields directly. While this works, it's worth considering the impact on encapsulation and testability.
Verify that the ProgramSetting class has been updated to access these static fields directly, and consider if this design choice might affect testability.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Pattern suggestions ✂️ (1)You could add these patterns to If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs:246
- Wrap the _win32sLock acquisition in a try-finally block when loading cache data to guarantee that the lock is always released even if an exception occurs.
await _win32sLock.WaitAsync();
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
383-384: Ensures correct usage with MemoryPack serialization.The remarks section clearly indicates that objects must be MemoryPackSerializable, which is important information for plugin developers to know.
🧹 Nitpick comments (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
373-385: Good addition of async cache saving functionality.The method is well-documented and provides a clear way to save binary storage for plugin caches. I like that you've explained both manual and automatic saving behavior.
Consider adding error handling information to the documentation. It would be helpful to explicitly state what happens if an error occurs during saving (e.g., if the file system is not writable).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs(2 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/ISavable.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher.Plugin/Interfaces/ISavable.cs
- Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs
- Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧰 Additional context used
🧬 Code Definitions (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
Flow.Launcher/PublicAPIInstance.cs (2)
SavePluginCaches(378-385)T(241-248)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
SavePluginCaches(193-196)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (3)
354-357: Method signature looks good.The
SavePluginCaches()method is clearly documented and follows the same naming convention as the existing methods likeSavePluginSettings(). The method will provide a clean way to persist plugin caches.
359-371: Good addition of async cache loading functionality.The method is well-documented and provides a clear way to load binary storage for plugin caches. The async approach is appropriate for I/O operations and will help improve performance.
A few observations:
- The generic constraint
where T : new()ensures the type has a parameterless constructor- The documentation explains what happens when the cache file doesn't exist
- The remarks section properly explains the dependency on MemoryPack serialization
369-370: Ensures correct usage with MemoryPack serialization.The remarks section clearly indicates that objects must be MemoryPackSerializable, which is important information for plugin developers to know.
New API Functions for plugin cache storage
Since #3272 has brought plugin cache path, this PR follows on to add support for loading and saving plugin cache with these api functions.
Additionally,
Programplugin uses these functions and this PR converts those function into api functions.Additionally, we should use
ISavableinterface instead of reflection for calling JsonStorage & BinaryStorageSave()function and we should useIRemovableinterface instead of reflection for removing storage instances fromIPublicAPIinstance.Test
Programplugin loading & reading caches works.