Unify logging for core projects & Ignore plugin source http exception #3460
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 pull request refactors logging and error handling across multiple modules. Most changes replace direct static logging (e.g., using Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment (Python/TypeScript)
participant JTF as JoinableTaskFactory
participant Droplex as DroplexPackage
participant API as Logging API
Env->>JTF: Run(DroplexPackage.Drop)
alt Successful Execution
JTF-->>Env: Return Result
Env->>API: Log success (using ClassName)
else Exception Occurs
JTF-->>Env: Raise Exception
Env->>API: Log exception (using ClassName)
end
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: 2
♻️ Duplicate comments (1)
Flow.Launcher.Infrastructure/Storage/FlowLauncherJsonStorage.cs (1)
42-42: Changed from API-based logging to direct Log calls.Same observation as line 30 - the direction of change is opposite to other files in this PR.
🧹 Nitpick comments (3)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (1)
23-23: Consider using nameof operator for class name consistency.Using a hardcoded string "BinaryStorage" could lead to inconsistencies if the class name changes in the future. Consider using
nameof(BinaryStorage<T>)instead, which would automatically update if the class is renamed and match the pattern used in other files.- private static readonly string ClassName = "BinaryStorage"; + private static readonly string ClassName = nameof(BinaryStorage<T>);Flow.Launcher.Infrastructure/Storage/JsonStorage.cs (2)
19-20: Good addition of class name constant for logging with minor consideration.Using a string literal "JsonStorage" instead of nameof() is reasonable for a generic class, as nameof would produce "JsonStorage`1". However, this approach loses the refactoring safety that nameof() provides.
Consider using a clearer name like "JsonStorageGeneric" to indicate this is for the generic class, or you could use
nameof(JsonStorage<T>).Split('')[0]` to get "JsonStorage" programmatically.
109-109: Improved consistency in logging.Using the ClassName variable instead of a hardcoded string improves consistency and maintainability.
Note that unlike other files in this PR, you're still using direct Log.Info instead of an API abstraction. Consider standardizing this approach across all files if appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
Flow.Launcher.Core/Configuration/Portable.cs(3 hunks)Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs(2 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs(2 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/JavaScriptEnvironment.cs(0 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/JavaScriptV2Environment.cs(0 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs(2 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs(2 hunks)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs(2 hunks)Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs(2 hunks)Flow.Launcher.Core/Plugin/PluginConfig.cs(5 hunks)Flow.Launcher.Core/Plugin/PluginManager.cs(5 hunks)Flow.Launcher.Core/Plugin/ProcessStreamPluginV2.cs(2 hunks)Flow.Launcher.Core/Resource/Internationalization.cs(6 hunks)Flow.Launcher.Core/Resource/Theme.cs(7 hunks)Flow.Launcher.Core/Updater.cs(4 hunks)Flow.Launcher.Infrastructure/Http/Http.cs(8 hunks)Flow.Launcher.Infrastructure/Image/ImageLoader.cs(6 hunks)Flow.Launcher.Infrastructure/Logger/Log.cs(0 hunks)Flow.Launcher.Infrastructure/Stopwatch.cs(2 hunks)Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs(3 hunks)Flow.Launcher.Infrastructure/Storage/FlowLauncherJsonStorage.cs(3 hunks)Flow.Launcher.Infrastructure/Storage/JsonStorage.cs(2 hunks)Flow.Launcher.Infrastructure/Storage/PluginBinaryStorage.cs(3 hunks)Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs(3 hunks)Flow.Launcher/App.xaml.cs(6 hunks)Flow.Launcher/Converters/QuerySuggestionBoxConverter.cs(2 hunks)Flow.Launcher/Helper/AutoStartup.cs(7 hunks)Flow.Launcher/Helper/HotKeyMapper.cs(4 hunks)Flow.Launcher/Helper/WallpaperPathRetrieval.cs(5 hunks)Flow.Launcher/MessageBoxEx.xaml.cs(2 hunks)Flow.Launcher/Msg.xaml.cs(0 hunks)Flow.Launcher/Notification.cs(2 hunks)Flow.Launcher/ProgressBoxEx.xaml.cs(2 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Flow.Launcher/ReportWindow.xaml.cs(2 hunks)Flow.Launcher/SettingWindow.xaml.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(5 hunks)Flow.Launcher/ViewModel/ResultViewModel.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs(2 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(0 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Baidu.cs(3 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Bing.cs(2 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/DuckDuckGo.cs(2 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Google.cs(2 hunks)Plugins/Flow.Launcher.Plugin.WindowsSettings/Log.cs(1 hunks)
💤 Files with no reviewable changes (5)
- Flow.Launcher.Core/ExternalPlugins/Environments/JavaScriptV2Environment.cs
- Flow.Launcher/Msg.xaml.cs
- Flow.Launcher.Core/ExternalPlugins/Environments/JavaScriptEnvironment.cs
- Plugins/Flow.Launcher.Plugin.Program/Main.cs
- Flow.Launcher.Infrastructure/Logger/Log.cs
🧰 Additional context used
🧬 Code Graph Analysis (17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
LogException(472-475)
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (4)
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (1)
InstallEnvironment(36-45)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (1)
InstallEnvironment(33-40)Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (1)
InstallEnvironment(132-132)Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs (2)
FilesFolders(14-367)RemoveFolderIfExists(115-131)
Flow.Launcher/Helper/HotKeyMapper.cs (2)
Flow.Launcher/App.xaml.cs (2)
App(27-360)App(51-115)Flow.Launcher/PublicAPIInstance.cs (1)
LogError(216-217)
Flow.Launcher/ProgressBoxEx.xaml.cs (2)
Flow.Launcher/App.xaml.cs (2)
App(27-360)App(51-115)Flow.Launcher/PublicAPIInstance.cs (1)
LogError(216-217)
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (4)
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (1)
InstallEnvironment(36-45)Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (1)
InstallEnvironment(33-40)Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (1)
InstallEnvironment(132-132)Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs (2)
FilesFolders(14-367)RemoveFolderIfExists(115-131)
Flow.Launcher.Core/Plugin/PluginManager.cs (4)
Flow.Launcher/PublicAPIInstance.cs (2)
LogInfo(210-211)LogException(219-220)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
LogInfo(256-256)LogException(272-272)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
LogInfo(148-151)Flow.Launcher.Infrastructure/Logger/Log.cs (2)
Exception(104-114)Exception(164-182)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
LogException(472-475)
Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Bing.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
Main(16-257)Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
LogException(472-475)
Flow.Launcher/ReportWindow.xaml.cs (1)
Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs (1)
DataLocation(6-43)
Flow.Launcher/ViewModel/ResultViewModel.cs (2)
Flow.Launcher/ViewModel/ResultsViewModel.cs (3)
ResultsViewModel(16-350)ResultsViewModel(26-30)ResultsViewModel(32-48)Flow.Launcher/PublicAPIInstance.cs (1)
LogException(219-220)
Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs (5)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
Main(16-257)Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
Main(19-462)Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1)
Main(16-450)Plugins/Flow.Launcher.Plugin.Sys/Main.cs (1)
Main(19-516)Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
LogException(472-475)
Flow.Launcher/Converters/QuerySuggestionBoxConverter.cs (2)
Flow.Launcher/App.xaml.cs (2)
App(27-360)App(51-115)Flow.Launcher/PublicAPIInstance.cs (1)
LogException(219-220)
Flow.Launcher.Core/Resource/Internationalization.cs (1)
Flow.Launcher/PublicAPIInstance.cs (2)
LogError(216-217)LogException(219-220)
Flow.Launcher/PublicAPIInstance.cs (2)
Flow.Launcher.Infrastructure/Stopwatch.cs (1)
Stopwatch(8-59)Flow.Launcher.Infrastructure/Logger/Log.cs (4)
Debug(204-207)Debug(210-213)Info(215-218)Info(221-224)
Flow.Launcher.Core/Resource/Theme.cs (2)
Flow.Launcher/PublicAPIInstance.cs (3)
LogError(216-217)LogException(219-220)LogWarn(213-214)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (3)
LogError(266-266)LogException(272-272)LogWarn(261-261)
Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs (5)
Flow.Launcher.Infrastructure/Http/Http.cs (2)
Http(15-217)Http(23-32)Flow.Launcher.Core/Plugin/PluginConfig.cs (2)
List(25-65)List(67-103)Flow.Launcher.Core/Plugin/PluginManager.cs (1)
List(352-379)Flow.Launcher.Core/Resource/Theme.cs (1)
List(396-409)Plugins/Flow.Launcher.Plugin.PluginsManager/Utilities.cs (1)
UserPlugin(62-80)
Flow.Launcher.Infrastructure/Stopwatch.cs (1)
Flow.Launcher.Infrastructure/Logger/Log.cs (6)
Debug(204-207)Debug(210-213)Log(13-230)Log(19-64)Info(215-218)Info(221-224)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
🔇 Additional comments (143)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (2)
12-13: Good practice: Added class name constant for consistent logging.Using a constant for the class name provides consistency and easier maintenance across logging calls.
49-49: Improved logging with API-based approach.Replaced direct logging with dependency injection-based logging using IoC container. This aligns with the PR objective of standardizing logging across the application.
However, I don't see changes that specifically address the PR objective of "Ignore Plugin Source HTTP Exception". The code still catches and logs all exceptions without special handling for HTTP-specific exceptions.
Is there a specific reason why HTTP exceptions aren't being handled differently here, given the PR objective? Or is this implemented elsewhere?
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (2)
18-18: Good addition of the ClassName field for better logging maintainability.Adding a static readonly field for the class name is a good practice that centralizes the class identifier for logging, making future refactoring easier.
474-474: LGTM: Improved logging with ClassName field reference.This change uses the centralized ClassName field for logging exceptions, which is consistent with the PR's goal of standardizing logging practices.
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs (3)
8-8: Good addition of Microsoft.VisualStudio.Threading namespace.This namespace is required for using the JoinableTaskFactory which enables non-blocking asynchronous operations.
31-31: Good addition of JoinableTaskFactory property.Adding the JTF property with a new JoinableTaskContext enables asynchronous execution of potentially blocking operations, which is important for maintaining UI responsiveness.
37-37:❓ Verification inconclusive
Improved async handling for package installation.
Replacing the direct call to DroplexPackage.Drop with JTF.Run prevents blocking the UI thread, which is important for maintaining application responsiveness during environment installation.
This change ensures consistency with similar implementations in TypeScriptEnvironment.cs and PythonEnvironment.cs. Consider adding error handling here to catch any exceptions during the asynchronous operation:
🏁 Script executed:
#!/bin/bash # Check for error handling in other environment classes rg -A 5 "JTF.Run" --glob "Flow.Launcher.Core/ExternalPlugins/Environments/*.cs"Length of output: 2126
Async handling is improved, but consider adding error handling for robustness.
Replacing the direct call withJTF.Run(() => DroplexPackage.Drop(App.nodejs_16_18_0, InstallPath));prevents UI thread blocking, which is great. However, note that similar implementations inPythonEnvironment.csandTypeScriptEnvironment.csalso lack error handling. It’s advisable to wrap the asynchronous call in a try/catch block to capture any exceptions that may occur, ensuring that installation failures are logged or handled gracefully.
- File:
Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptV2Environment.cs(around line 37)- Suggestion: Consider updating the code to something like:
JTF.Run(() => { try { DroplexPackage.Drop(App.nodejs_16_18_0, InstallPath); } catch (Exception ex) { // TODO: handle error (e.g., log the exception) } });Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Bing.cs (3)
13-13: Good addition of the ClassName field for better logging maintainability.Adding a static readonly field for the class name is a good practice that centralizes the class identifier for logging, making future refactoring easier.
38-38: LGTM: Improved exception logging with ClassName field reference.This change uses the centralized ClassName field for logging HTTP request exceptions, which is consistent with the PR's goal of standardizing logging practices.
43-43: LGTM: Improved JSON exception logging with ClassName field reference.This change uses the centralized ClassName field for logging JSON parsing exceptions, which is consistent with the PR's goal of standardizing logging practices.
Flow.Launcher/SettingWindow.xaml.cs (1)
49-49: Updated to use static API instance instead of injected field.This change aligns with the PR's objective of standardizing access to API services by using the static
App.APIproperty instead of an injected_apifield, maintaining functionality while improving consistency across the codebase.Flow.Launcher.Core/Resource/Theme.cs (7)
27-28: Good addition of a consistent class identifier for logging.Adding the ClassName variable provides better maintainability and consistency for logging across the codebase.
77-77: Improved logging with contextual information.Replacing direct Log.Error calls with API-based logging that includes class context enhances log traceability.
96-97: Enhanced exception logging with class context.The refactored logging method properly includes the class name, providing better context for troubleshooting.
139-140: Consistent logging approach applied to exception handling.The change maintains the same error handling logic while improving the logging implementation.
391-392: Standardized warning logging with class context.Using the class variable instead of a hardcoded string improves maintainability.
444-445: Consistent error logging in exception handlers.The approach is consistent with the PR's objective to standardize logging across the application.
454-455: Standardized error logging in XamlParseException handler.This change completes the consistent logging approach throughout the file.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
18-19: Good addition of class identifier for consistent logging.Using a static readonly field for the class name enhances maintainability and follows the standardized logging approach being implemented across the codebase.
226-226: Improved exception logging with class context.Replacing the hardcoded class name with the ClassName variable improves maintainability while keeping the same error handling logic.
Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Google.cs (3)
13-14: Good addition of class identifier for consistent logging.Adding the ClassName variable aligns with the standardized logging approach being implemented across the codebase.
32-33: Enhanced exception logging for HTTP requests.The change improves maintainability while maintaining the same error handling logic for HTTP request exceptions.
37-38: Consistent approach to exception logging for JSON parsing.The change maintains the error handling logic while improving the logging implementation.
Flow.Launcher/MessageBoxEx.xaml.cs (2)
12-13: Good addition of class identifier for consistent logging.Adding the ClassName variable aligns with the standardized logging approach being implemented across the codebase.
62-63: Improved error logging with API-based approach.Replacing direct Log.Error calls with App.API.LogError provides better context through the class name and aligns with the PR's objective to standardize logging.
Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/DuckDuckGo.cs (3)
13-14: Good refactoring for improved logging context.Adding a static
ClassNamevariable improves maintainability by centralizing the class name reference used in logging. Usingnameof(DuckDuckGo)ensures the variable automatically updates if the class is renamed.
39-39: LGTM!Updating the logging to use the
ClassNamevariable provides consistent context in logs.
44-44: LGTM!Similarly, using
ClassNamehere ensures consistent class naming in logs.Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (2)
64-64: LGTM!The log statement now uses the centralized
ClassNamevariable for context.
74-74: LGTM!This log statement now also uses the
ClassNamevariable for consistency.Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Baidu.cs (3)
14-14: Good refactoring for improved logging context.Adding a static
ClassNamevariable improves maintainability by centralizing the class name reference used in logging. Usingnameof(Baidu)ensures the variable automatically updates if the class is renamed.
29-29: LGTM!Updating the logging to use the
ClassNamevariable provides consistent context in logs.
44-44: LGTM!Similarly, using
ClassNamehere ensures consistent class naming in logs.Flow.Launcher/Helper/HotKeyMapper.cs (4)
15-15: Good refactoring for improved logging context.Adding a static
ClassNamevariable improves maintainability by centralizing the class name reference used in logging. Usingnameof(HotKeyMapper)ensures the variable automatically updates if the class is renamed.
56-60: Improved logging through API abstraction.Good change from direct
Log.Errorcalls to using the abstractedApp.API.LogErrormethod. This is in line with the PR objective of standardizing logging practices across the application. The API approach provides more flexibility for future enhancements to logging.
81-85: LGTM!Similarly, updating to use
App.API.LogErrorwith theClassNamevariable provides consistent logging context.
107-111: LGTM!Consistent with other changes, using
App.API.LogErrorwith theClassNamevariable improves logging structure.Flow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.cs (3)
8-8: Added necessary import for JoinableTaskFactory.The import for Microsoft.VisualStudio.Threading is required for the JoinableTaskFactory implementation.
31-32: Good addition of the JoinableTaskFactory property.This property follows the pattern implemented in other environment classes (PythonEnvironment and TypeScriptV2Environment) and facilitates asynchronous execution.
37-37:✅ Verification successful
Improved responsiveness by making installation non-blocking.
Changed from a blocking call to using JTF.Run(), which improves application responsiveness during potentially long-running operations like package installations.
Verify that subsequent operations don't rely on the installation being completed:
🏁 Script executed:
#!/bin/bash # Check if code uses any awaits or expects the installation to be completed rg -A 5 -B 5 "InstallEnvironment" --type csharpLength of output: 7923
Non-Blocking Installation Confirmed
The use of
JTF.Run()inFlow.Launcher.Core/ExternalPlugins/Environments/TypeScriptEnvironment.csnow properly triggers the package installation asynchronously, thereby improving responsiveness. Our grep check across the codebase confirms that subsequent operations invokingInstallEnvironment()do not await or depend on its completion. No additional changes are required.Flow.Launcher/ProgressBoxEx.xaml.cs (2)
10-11: Good addition of class name constant for logging.Using nameof(ProgressBoxEx) ensures that if the class name changes during refactoring, this reference will be automatically updated, improving maintainability.
51-51: Standardized logging approach.Changed from using direct Log.Error call to App.API.LogError, which aligns with the PR objective of standardizing logging across the application. Including the class name as context improves traceability.
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (2)
16-17: Good addition of class name constant for logging.Using nameof(AbstractPluginEnvironment) ensures that if the class name changes during refactoring, this reference will be automatically updated, improving maintainability.
124-126: Standardized logging approach.Changed from using direct Log.Error call to API.LogError, which aligns with the PR objective of standardizing logging across the application. Including the class name as context improves traceability while preserving the same error message content.
Flow.Launcher.Core/Plugin/PluginConfig.cs (9)
8-8: Added dependency for IoC container.This import adds the dependency injection capability needed for the API service retrieval.
14-14: Good practice: Added class name constant for logging.Using a constant for the class name in logs improves consistency and maintainability.
16-18: Implemented lazy loading for API service.The comment correctly explains why static constructor initialization is avoided. The lazy initialization pattern ensures the API instance is only created when needed.
41-41: Replaced direct Log call with API-based logging.Changed from using static
Log.ExceptiontoAPI.LogExceptionwith the new class name constant.
58-62: Refactored warning logging to use API.Updated to use
API.LogWarnwith the class name constant for consistent logging approach.
110-110: Refactored error logging to use API.Updated to use
API.LogErrorwith the class name constant.
126-126: Refactored exception logging to use API.Updated to use
API.LogExceptionwith the class name constant.
132-132: Refactored error logging to use API.Updated to use
API.LogErrorwith the class name constant.
138-138: Refactored error logging to use API.Updated to use
API.LogErrorwith the class name constant.Flow.Launcher.Infrastructure/Storage/FlowLauncherJsonStorage.cs (2)
3-3: Updated import to include Logger namespace.Added explicit reference to the Logger namespace for using the Log class directly.
30-30:✅ Verification successful
Changed from API-based logging to direct Log calls.
This change appears to be opposite to the pattern seen in other files. In most other files, direct
Logcalls are being replaced withAPI.Logcalls, while here it's the reverse.
🏁 Script executed:
#!/bin/bash # Check if there's a pattern of changing API-based logging to direct Log calls in infrastructure files echo "Checking if this pattern is consistent across infrastructure files..." rg -A 1 "Log\.Exception" --glob="Flow.Launcher.Infrastructure/**/*.cs" echo "---------------" echo "Checking if there's a transition from API.LogException to Log.Exception..." rg -A 1 -B 1 "API\.LogException" --glob="Flow.Launcher.Infrastructure/**/*.cs"Length of output: 3617
Logging Pattern Consistency Verified
After checking the codebase, all infrastructure files (including the ones in Http, Storage, and Image folders) consistently use direct
Log.Exceptioncalls. There are no instances ofAPI.LogException, so the change inFlowLauncherJsonStorage.csfollows the established pattern. No modifications are needed here.Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs (2)
12-12: Good practice: Added class name constant for logging.Using a constant for the class name in logs improves consistency and maintainability across the codebase.
80-80: Updated logging to use class name constant.Updated to use the new
ClassNameconstant instead of hardcoding the class name in logging calls.Flow.Launcher.Core/Plugin/PluginManager.cs (6)
216-217: Refactored info logging to use API.Updated from
Log.InfotoAPI.LogInfoto maintain consistent logging approach.
221-221: Refactored exception logging to use API.Updated from
Log.ExceptiontoAPI.LogExceptionfor consistent logging approach.
372-374: Refactored exception logging to use API.Updated from
Log.ExceptiontoAPI.LogExceptionwith improved formatting for better readability.
565-565: Refactored exception logging to use API.Updated from
Log.ExceptiontoAPI.LogException.
610-610: Refactored exception logging to use API.Updated from
Log.ExceptiontoAPI.LogException.
626-626: Refactored exception logging to use API.Updated from
Log.ExceptiontoAPI.LogException.Flow.Launcher/Helper/AutoStartup.cs (7)
14-14: LGTM: Good addition of a standardized way to reference class name.Adding a static readonly variable for the class name is a good practice for maintainability and consistency in logging statements.
39-39: LGTM: Good switch to API-based logging.Switched from direct
Log.ErrortoApp.API.LogError, which matches the PR objective of standardizing logging across the application.
66-66: LGTM: Consistent use of API-based logging.Consistently applying the logging improvement pattern here.
117-117: LGTM: Maintaining consistency in error logging.All error logging has been standardized to use the
App.API.LogErrormethod.
138-138: LGTM: Consistent logging pattern applied.Continues the pattern of standardized logging with class name context.
166-166: LGTM: Standardized error logging.Standardized error logging with proper context.
181-181: LGTM: Final logging standardization applied.All logging calls now consistently use the API-based approach.
Flow.Launcher.Core/Plugin/ProcessStreamPluginV2.cs (2)
10-10: LGTM: Enabling nullable reference types.Enabling nullable reference types improves type safety by helping detect possible null reference exceptions at compile time.
16-16: LGTM: Improved field declaration.The change from
private static JobObject _jobObject = new JobObject();toprivate static readonly JobObject _jobObject = new();has two improvements:
- Added
readonlymodifier to prevent accidental reassignment- Using the modern C# target-typed new expression for cleaner syntax
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (6)
10-10: LGTM: Simplified dependency in Infrastructure project.Added direct import of the Logger namespace instead of relying on IPublicAPI for logging functions, which aligns with the PR objective of having the Infrastructure project use static logging.
56-63: LGTM: Switched from API-based to direct logging.Replaced
API.StopwatchLogInfoAsyncwithStopwatch.InfoAsyncandAPI.LogInfowithLog.Info, which aligns with the PR objective of standardizing logging for the Infrastructure project to use static logging.
79-79: LGTM: Consistent direct logging in Infrastructure.Changed from API-based exception logging to direct static logging, maintaining consistency in the Infrastructure project.
174-175: LGTM: Consistent exception logging.Applying the same logging pattern consistently throughout the class.
241-241: LGTM: Consistently using direct logging.Maintained consistency in the error logging approach.
265-265: LGTM: Final consistent logging change.All logging calls consistently use direct static logging now.
Flow.Launcher/Converters/QuerySuggestionBoxConverter.cs (2)
13-13: LGTM: Good addition of class name constant.Adding a static readonly variable for the class name is a good practice for maintainability and consistency in logging statements.
68-68: LGTM: Switched to API-based logging.Replaced direct
Log.ExceptionwithApp.API.LogException, which aligns with the PR objective of standardizing non-Infrastructure projects to use API-based logging.Flow.Launcher.Infrastructure/Storage/PluginBinaryStorage.cs (3)
3-3: Updated import to use infrastructure Logger.This import change aligns with the PR objective to standardize logging across the application, shifting from API-based logging to the static Log class.
10-10: Added class identifier for consistent logging.A static class name field has been added to provide a consistent identifier for logging, improving traceability of log entries.
28-28: Replaced API-based logging with static Log class.The code now uses
Log.Exceptionwith theClassNameparameter instead of the previousAPI.LogException. This change is consistent with the PR objective to standardize logging practices.Also applies to: 40-40
Flow.Launcher.Core/Configuration/Portable.cs (3)
1-12: Updated and reorganized imports.The imports have been reorganized and updated to include necessary dependencies while removing unused ones.
18-19: Added class identifier for consistent logging.A static class name field has been added using
nameof(Portable)to provide a consistent identifier for logging, improving traceability of log entries.
55-55: Switched to API-based logging with class identifier.Changed from using
Log.ExceptiontoAPI.LogExceptionwith theClassNameparameter. While this moves in the opposite direction compared to PluginBinaryStorage.cs (which moved from API to static logging), it maintains consistency within this class as it already has access to the API instance.Also applies to: 79-79
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (2)
15-16: Added class identifier for consistent logging.A static class name field has been added using
nameof(ProcessHelper)to provide a consistent identifier for logging, improving code maintainability.
136-136: Updated logging to use class identifier variable.The code now uses the
ClassNamevariable instead of an inlinenameof(ProcessHelper)expression, improving consistency and maintainability.Flow.Launcher/Helper/WallpaperPathRetrieval.cs (2)
15-16: Added class identifier for consistent logging.A static class name field has been added using
nameof(WallpaperPathRetrieval)to provide a consistent identifier for logging, improving code maintainability.
34-34: Updated multiple logging calls to use class identifier variable.All logging statements now use the
ClassNamevariable instead of inlinenameof(WallpaperPathRetrieval)expressions. This improves consistency and maintainability by centralizing the class name reference to a single location.Also applies to: 59-59, 100-100, 118-118
Flow.Launcher.Infrastructure/Storage/PluginJsonStorage.cs (2)
3-3: Updated import for logging.The import statement has been added to access the Logger functionality directly, which aligns with the PR's goal of standardizing logging across the application.
40-40: Improved exception logging with consistent class name reference.The exception logging has been updated to use
Log.Exception(ClassName, ...)which provides better context in logs by including the class name. This change aligns with the PR objective to standardize logging across the application by moving from API-based logging to direct Log class usage.Also applies to: 52-52
Flow.Launcher/ViewModel/ResultViewModel.cs (1)
235-237: Improved exception logging through API.The exception logging has been updated to use
App.API.LogExceptionwith the class name parameter, which provides better context in logs. This change aligns with the PR objective to standardize logging across the application.Flow.Launcher/ReportWindow.xaml.cs (3)
12-12: Added import for DataLocation.The import statement has been added to access the DataLocation functionality, which is now used to retrieve the log directory path.
18-18: Added class name constant for logging.A static readonly string field
ClassNamehas been added to store the class name, which is a good practice for consistent logging across the application.
43-43: Updated log directory source.Changed from using
Log.CurrentLogDirectorytoDataLocation.VersionLogDirectoryfor retrieving the log directory path. This centralizes the log directory path management in the DataLocation class.Flow.Launcher.Core/Updater.cs (3)
26-26: Added class name constant for logging.A static readonly string field
ClassNamehas been added, which improves logging consistency by providing context to log messages.
55-55: Standardized info logging through API.Updated logging calls to use
_api.LogInfo(ClassName, ...)instead of direct logging, which aligns with the PR objective to standardize logging practices.Also applies to: 88-88
97-104: Improved exception handling and logging.The exception handling has been updated with:
- Modern C# pattern matching for exception types using the
orkeyword for better readability- Standardized logging through
_api.LogException(ClassName, ...)- Better error message for network-related exceptions
These changes align with the PR objective to improve error handling for HTTP exceptions while standardizing logging.
Flow.Launcher/App.xaml.cs (7)
151-152: Improved logging with standardized context pattern.The logging has been updated to use
API.LogInfowith a consistentClassNamevariable instead of directly usingLog.Infowith hardcoded strings. This matches the PR objective of standardizing the logging mechanism across the application.
179-179: Standardized logging format for dependencies info.Using
API.LogInfowithClassNamefor logging dependencies information ensures consistent logging patterns throughout the application.
195-195: Standardized logging for end of startup process.This follows the same pattern of using
API.LogInfowithClassNamefor consistency in logs.
253-253: Standardized logging for process exit events.Updated exit event logging to use
API.LogInfowithClassNamefor better consistency and context in logs.
259-259: Standardized logging for application exit events.Updated to use the new logging pattern consistent with other changes.
265-265: Standardized logging for session ending events.Updated to use the new logging pattern consistent with other changes.
329-329: Standardized logging for application disposal process.Updated both the beginning and end of the disposal process logs to use the new
API.LogInfowithClassNamepattern.Also applies to: 339-339
Flow.Launcher/ViewModel/MainViewModel.cs (4)
32-33: Added class identifier for consistent logging.Added a private static readonly field
ClassNameto store the class name for use in logging calls. This improves maintainability by centralizing the class name reference and ensuring consistency across all log messages.
217-217: Updated error logging to use API with class context.Changed from direct
Log.ErrortoApp.API.LogErrorwith theClassNamevariable to provide better context in logs for unexpected result view updates.
261-261: Updated queue error logging to use API with class context.Changed direct logging to use
App.API.LogErrorwithClassNamefor more consistent error logging.
351-353: Updated exception logging to use API with class context.Changed from direct exception logging to using
App.API.LogExceptionwithClassNameand improved the error message format for better context when shortcut expansion fails.Flow.Launcher.Core/Resource/Internationalization.cs (6)
19-24: Implemented lazy API initialization for logging.This change:
- Adds a private static readonly field
ClassNamefor consistent logging- Uses lazy initialization for the API to prevent circular dependencies
- Includes a detailed comment explaining why API shouldn't be initialized in the static constructor
This is a good approach to manage dependencies while ensuring consistent logging.
88-88: Updated plugin path error logging to use API.Changed from direct logging to use
API.LogErrorwithClassNamefor more consistent error reporting when plugin paths cannot be found.
158-158: Updated language code error logging to use API.Changed from direct logging to use
API.LogErrorwithClassNamefor more consistent error reporting when language codes cannot be found.
256-256: Updated translation key error logging to use API.Changed from direct logging to use
API.LogErrorwithClassNamefor more consistent error reporting when translation keys cannot be found.
274-274: Updated plugin exception logging to use API.Changed from direct exception logging to use
API.LogExceptionwithClassNamefor more consistent error reporting when plugin translations fail.
290-290: Updated language path error logging to use API.Changed from direct logging to use
API.LogErrorwithClassNamefor more consistent error reporting when language paths cannot be found.Also applies to: 298-298
Flow.Launcher.Infrastructure/Http/Http.cs (6)
1-12: Improved code organization and readability.The using directives have been reorganized for better clarity, and unnecessary empty lines have been removed to improve code readability.
17-18: Added class identifier for consistent logging.Added a static field
ClassNamefor use in logging calls throughout the class, improving the consistency of log messages.
21-21: Improved HttpClient declaration.Changed the HttpClient declaration to:
- Make it readonly to prevent accidental reassignment
- Use the C# 9.0 target-typed new expression syntax (
new())This is a good improvement for both safety and modern C# practices.
38-38: Simplified property getter.Changed the property getter from a block body to an expression-bodied member, making the code more concise without changing functionality.
76-76: Updated exception creation in switch expression.Changed the
ArgumentOutOfRangeExceptionto include a null argument. This is a minor change that aligns with .NET guidelines for creating exceptions.
82-82: Updated logging to use consistent class context.All logging calls now use the
ClassNamevariable instead of hardcoded strings. This improves maintainability by ensuring all logs from this class have consistent identifiers.Also applies to: 138-138, 151-151, 163-163, 195-195, 206-206
Flow.Launcher/PublicAPIInstance.cs (4)
437-438: Implementation updated to use the new API format.The
StopwatchLogDebugmethod now correctly passes parameters to the updatedStopwatch.Debugmethod, matching its new signature that accepts class name and method name as separate parameters rather than a formatted string.
440-441: Implementation updated to use the new API format.The
StopwatchLogDebugAsyncmethod now correctly passes parameters to the updatedStopwatch.DebugAsyncmethod, aligning with the new parameter structure.
443-444: Implementation updated to use renamed method and new parameter format.The
StopwatchLogInfomethod now correctly callsStopwatch.Info(previously calledNormal) with the new parameter structure, improving consistency in the logging API.
446-447: Implementation updated to use renamed method and new parameter format.The
StopwatchLogInfoAsyncmethod now correctly callsStopwatch.InfoAsync(previously calledNormalAsync) with the new parameter structure, completing the standardization of logging method calls.Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs (6)
19-22: Good implementation of lazy initialization for API service.The static API property with lazy initialization through dependency injection is a good pattern that avoids circular dependencies. The comment explains why API isn't initialized in the static constructor, which is helpful for future maintainers.
23-23: Added consistent class name variable for logging.Adding a static readonly
ClassNamefield follows good practice for standardized logging across the application, making the class name consistent throughout all log messages.
29-29: Improved field declaration with readonly modifier and target-typed new.The field is now correctly marked as readonly and uses the more concise target-typed new operator, improving both thread safety and readability.
44-44: Updated logging to use API interface.Logging now correctly uses the API interface with the standardized ClassName variable, which aligns with the PR objective to shift from static Log class to API-based logging.
50-75: Improved HTTP response handling with comprehensive status code checks.The HTTP response handling has been significantly improved with specific handling for different status codes:
- OK (200): Deserializes content, updates ETag, and logs success
- NotModified (304): Logs the unchanged status and returns cached plugins
- Other status codes: Logs a warning and returns cached plugins
This enhanced error handling ensures more reliable operation and better diagnostic information.
76-87: Enhanced exception handling with specific connection-related error messages.This exception handling block now correctly:
- Specifically identifies network-related exceptions (HttpRequestException, WebException, SocketException, TimeoutException)
- Provides a more helpful error message for connection issues
- Logs general exceptions separately
- Always returns the cached plugins list to maintain consistent return type
This implementation aligns with the PR objective to "Ignore Plugin Source HTTP Exception" while still providing informative logs.
Flow.Launcher.Infrastructure/Stopwatch.cs (5)
2-2: Added required namespace for CallerMemberName attribute.The System.Runtime.CompilerServices namespace has been correctly added to support the CallerMemberName attribute used in the updated method signatures.
13-21: Updated Debug method signature and implementation to include class and method context.The Debug method signature has been enhanced to:
- Accept className and methodName parameters
- Use CallerMemberName attribute for automatic method name capture
- Pass these parameters to Log.Debug for improved logging context
This change improves the logging by providing more structured information about where the timed action is occurring.
27-35: Updated DebugAsync method signature and implementation with consistent context parameters.Similar to the synchronous version, the DebugAsync method now accepts and properly uses className and methodName parameters, ensuring consistent logging across both synchronous and asynchronous operations.
38-47: Renamed Normal method to Info with updated signature for better clarity.The method has been appropriately renamed from Normal to Info, which better reflects the logging level it uses (Log.Info). The signature has also been updated to include className and methodName parameters consistent with the Debug methods.
49-57: Renamed NormalAsync method to InfoAsync with updated signature for consistent naming.The asynchronous counterpart has been renamed from NormalAsync to InfoAsync, maintaining naming consistency with the synchronous version while adding the same contextual parameters for improved logging.
Plugins/Flow.Launcher.Plugin.WindowsSettings/Log.cs (1)
14-14: Improved method spacing enhances code readabilityThe addition of blank lines between method definitions improves code readability and follows common coding conventions for C#.
Also applies to: 19-19
Flow.Launcher.Core/ExternalPlugins/Environments/PythonEnvironment.cs (3)
8-8: Added Microsoft.VisualStudio.Threading namespace for JoinableTaskFactoryThis namespace addition supports the new asynchronous execution implementation.
34-35: Added JoinableTaskFactory for improved asynchronous executionGood introduction of JoinableTaskFactory to replace blocking calls with asynchronous execution. This helps maintain UI responsiveness during potentially long-running operations.
42-42: Replaced blocking call with JTF.Run for asynchronous executionThe change from a likely direct await to JTF.Run() prevents potential UI freezing while maintaining synchronous behavior from the caller's perspective.
Flow.Launcher/Notification.cs (2)
11-12: Added class name constant for improved logging contextUsing nameof(Notification) for the class name is a good practice as it avoids hardcoded strings and ensures the class name stays in sync with any refactoring.
55-55: Standardized exception logging with API interfaceReplaced direct logging calls with App.API.LogException and included the class name for better context in logs. This change aligns with the PR objective to standardize logging practices across the application.
Also applies to: 60-60
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/Plugin/PluginManager.cs:216
- The variable 'ClassName' is referenced in the logging call without being declared in this file. Define it (e.g. 'private static readonly string ClassName = nameof(PluginManager);') to avoid runtime issues.
API.LogInfo(ClassName, $"Total init cost for <{pair.Metadata.Name}> is <{pair.Metadata.InitTime}ms>");
This comment has been minimized.
This comment has been minimized.
Jack251970
added
Code Quality
Dev branch only
@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 ...
|
|
🥷 Code experts: no user but you matched threshold 10 Jack251970, jjw24 have most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Core/Plugin/PluginsLoader.cs(1 hunks)Flow.Launcher.Infrastructure/Logger/Log.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(6 hunks)Flow.Launcher/ViewModel/ResultViewModel.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/ViewModel/ResultViewModel.cs
- Flow.Launcher/ViewModel/MainViewModel.cs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
Flow.Launcher.Infrastructure/Logger/Log.cs (3)
131-138: Improved exception handling separation between debug and release builds.The explicit preprocessor directive for the
ExceptionInternalmethod ensures that exceptions are handled differently in debug versus release builds. In debug mode, exceptions are rethrown for immediate debugging, while in release builds they're properly logged but not rethrown.This change is part of a broader refactoring to improve logging across the application by requiring contextual information (className, methodName) for all log entries, which aligns with the PR objective to standardize logging practices.
97-107: Standardized logging with improved context information.The removal of simplified logging methods (those taking only message parameters) enforces a more structured approach to logging throughout the codebase. Now all logging must include:
- Class name - providing source context
- Message - the actual log information
- Method name - automatically captured using CallerMemberName
This change aligns with modern logging best practices where contextual information is crucial for effective troubleshooting. The PR objective mentioned shifting from direct static Log class usage to an API-based approach, and these changes support that transition by standardizing the logging interface.
Also applies to: 140-167
1-175: Well-structured logging system with good separation of concerns.The logging implementation follows good practices:
- Validates inputs with helpful error messages
- Captures contextual information (class and method names)
- Uses different handling strategies for debug vs release builds
- Implements async file target for better performance
- Properly demystifies exceptions for clearer stack traces
Note that while the PR objectives mention "Ignore Plugin Source HTTP Exception," that change is not present in this file and must be implemented elsewhere in the codebase.
| catch (Exception e) when (assembly == null) | ||
| { | ||
| Log.Exception($"|PluginsLoader.DotNetPlugins|Couldn't load assembly for the plugin: {metadata.Name}", e); | ||
| Log.Exception(ClassName, $"Couldn't load assembly for the plugin: {metadata.Name}", e); |
There was a problem hiding this comment.
Use API.LogException instead of Log.Exception for consistency.
The PR's objective is to standardize logging by using API-based methods rather than static Log calls. Though the ClassName and API variables are properly defined at the top of the file, this logging statement still uses the static method.
Modify the logging call as follows:
-Log.Exception(ClassName, $"Couldn't load assembly for the plugin: {metadata.Name}", e);
+API.LogException(ClassName, $"Couldn't load assembly for the plugin: {metadata.Name}", e);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Log.Exception(ClassName, $"Couldn't load assembly for the plugin: {metadata.Name}", e); | |
| API.LogException(ClassName, $"Couldn't load assembly for the plugin: {metadata.Name}", e); |
|
Look good to me and already tested. |
jjw24
changed the title
jjw24
added
bug
3 participants
Improve log
For all other projects, we use API to get log functions.
For Infrastructure project, we use Log static class to get log functions.
Ignore plugin source http exception
Ignore exceptions like: